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.

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

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

Closed
wants to merge 2 commits into from

Conversation

VadimZhestikov
Copy link
Contributor

Proposed changes

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

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 njs_object_iterate() and This cannot fail comment.
This is not true anymore and should be checked for error as well.

external/njs_fs_module.c Outdated Show resolved Hide resolved
src/njs_value.h Outdated Show resolved Hide resolved
src/njs_value.h Outdated Show resolved Hide resolved
src/njs_value.h Outdated Show resolved Hide resolved
src/njs_value.h Outdated Show resolved Hide resolved
src/njs_object.c Outdated Show resolved Hide resolved
src/njs_object.c Outdated Show resolved Hide resolved
src/njs_object.c Outdated Show resolved Hide resolved
src/njs_builtin.c Outdated Show resolved Hide resolved
src/njs_builtin.c Outdated Show resolved Hide resolved
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:

Removed built-in short string and reserve atom_id field.

@VadimZhestikov VadimZhestikov force-pushed the atomic_strings branch 2 times, most recently from 733208d to 1eb9e74 Compare September 4, 2024 20:14
src/njs_object.c Outdated Show resolved Hide resolved
src/njs_object.c Outdated Show resolved Hide resolved
src/njs_function.c Outdated Show resolved Hide resolved
src/njs_object.c Outdated Show resolved Hide resolved
@VadimZhestikov
Copy link
Contributor Author

I'll squash 'Macro NJS_DECLARE_PROP_HANDLER is used for Uint8ClampedArray.' later.

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 VadimZhestikov deleted the atomic_strings branch September 17, 2024 19:16
@VadimZhestikov VadimZhestikov restored the atomic_strings branch September 17, 2024 19:42
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