Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Atomic keys introduction for strings, symbols, small numbers. #785

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Atomic keys introduction for strings, symbols, small numbers. #785

wants to merge 7 commits into from

Conversation

VadimZhestikov
Copy link
Contributor

Proposed changes

(This PR is clone of unintentionally closed PR 777)

This is series of commits which introduce internal atomic representation for
strings, symbols and small numbers as keys for access object properties.

These commits are intended to speed up access to object properties.

Step 1:
Remove long strings from njs core to simplify code and make it more regular.
Reserve atom_id field in njs_value_t.

Notes:
a) Each commit is fully and successfully tested for regressions with all local tests;
b) This PR is draft for internal review, I'll extend it by additional commits as
they will be merged here from my hg sandbox.
c) As soon as last commit will be added/reviewed, draft status from this PR can be removed,
and PR applied then.

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes

@VadimZhestikov
Copy link
Contributor Author

Added njs_cnst.[ch]. It is intermediate commit, the hash itself and unique pre-atomized njs_cnst.vs._STRING_NAMES will be used in next commits.

xeioex
xeioex previously requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested commit log:

Added predefined strings hash.

See my patches that address issues/improvements found by me here https://github.com/xeioex/njs/tree/atomic_strings

auto/sources Outdated Show resolved Hide resolved
src/njs_builtin.c Show resolved Hide resolved
src/njs_builtin.c Outdated Show resolved Hide resolved
src/njs_builtin.c Show resolved Hide resolved
src/njs_cnst.c Outdated Show resolved Hide resolved
src/njs_cnst.c Outdated Show resolved Hide resolved
src/njs_cnst.c Outdated Show resolved Hide resolved
src/njs_cnst.c Outdated Show resolved Hide resolved
src/njs_cnst.h Outdated Show resolved Hide resolved
src/njs_cnst.h Outdated Show resolved Hide resolved
xeioex
xeioex previously requested changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the patch for the mentioned issues/improvements here 965d68f

src/njs_vm.h Outdated Show resolved Hide resolved
src/njs_predefined.c Outdated Show resolved Hide resolved
src/njs_predefined.h Outdated Show resolved Hide resolved
@VadimZhestikov VadimZhestikov dismissed xeioex’s stale review September 26, 2024 15:37

comments addressed

@VadimZhestikov VadimZhestikov force-pushed the atomic_strings_new branch 3 times, most recently from 58cd9f5 to 99a3a62 Compare October 2, 2024 00:02
@VadimZhestikov
Copy link
Contributor Author

squashed working commits to simplify history

@VadimZhestikov VadimZhestikov force-pushed the atomic_strings_new branch 2 times, most recently from 0d49cc0 to c329285 Compare October 11, 2024 18:37
@VadimZhestikov
Copy link
Contributor Author

force-push missed change for njs_global_this_prop_handler()

@VadimZhestikov
Copy link
Contributor Author

polished a bit change for njs_global_this_prop_handler()

@VadimZhestikov
Copy link
Contributor Author

It is incremental patch.

@VadimZhestikov
Copy link
Contributor Author

small reordering applied

@VadimZhestikov
Copy link
Contributor Author

Now atom_ids are used in the code. All local tests are passed. To be squashed later, I'll add optional stuff soon (additional optimizations, randomized atom_ids, etc)

@VadimZhestikov
Copy link
Contributor Author

VadimZhestikov commented Nov 5, 2024 •
edited
Loading

Series of working commits are squashed to simplify history. Need take a look at still missed inline optimizations and replacing 'njs_value_t name' by atom_id in njs_object_prop_t.

@VadimZhestikov
Reserved atom_id in njs_value_t.
15c089e 
To have enough space in njs_value_t this patch removed built-in short string.
The short strings were an optimization which will be less useful when
atomic strings are introduced.
@VadimZhestikov
Added predefined strings hash.
774aff8 
-- Use refs to njs_atom.v[sw]_* in the code.
-- wellknown symbols are now njs_atom.vw_*;
-- njs_atom.v* content is used to initialize values in njs_object_prop_t;
-- code do not depend on atom_ids values used in njs_atom.v*;
-- at code top level always used explicit names: njs_atom.v*;
-- some small code reductions.
@VadimZhestikov
Define vm->atom_hash* and manipualtion of them.
ebaa37d 
-- added njs_flathsh_alloc_copy(mp, to, from);
-- added atom_hash_* fields in njs_vm_t;
-- added njs_flathsh_alloc_copy(mp, to, from);
-- added vm_parent for using atom_hash from parent_vm for preload_object case;
-- added code for using correct atom_hash and atom_hash_shared in vm and cloned vm.
@VadimZhestikov
Use atom_hash* instead of keywords_hash in the code, atoms not used yet.
7087734
@VadimZhestikov
Introduce njs_atom_atomize_key().
3afdcf5
@VadimZhestikov
Use atom_ids in the code.
457909e 
-- use njs_atom_atomize_key();
-- remove symbol_generator;
-- remove NJS_SYMBOL_KNOWN_MAX;
-- use atom_ids in njs_object_hash_test();
-- use atom_id as key_hash;
-- remove NJS_*_HASH;
-- remove not used files.
@VadimZhestikov
Inline optimizations.
3810cb3
@VadimZhestikov
Copy link
Contributor Author

VadimZhestikov commented Nov 8, 2024 •
edited
Loading

Now performance is same as in my initial sandbox. Looking at replacing 'njs_value_t name' by atom_id in njs_object_prop_t.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xeioex xeioex xeioex left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VadimZhestikov @xeioex