Skip to content

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
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

xeioex
xeioex previously requested changes Sep 26, 2024
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

@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

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
Copy link
Contributor Author

VadimZhestikov commented Nov 8, 2024

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants