Skip to content

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

VadimZhestikov
Copy link
Contributor

@VadimZhestikov VadimZhestikov commented Feb 6, 2025

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.

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 VadimZhestikov changed the title Atomic strings new Atomic keys introduction for strings, symbols, small numbers. Feb 6, 2025
@VadimZhestikov VadimZhestikov force-pushed the atomic_strings_new branch 13 times, most recently from 269870d to d0dc432 Compare February 14, 2025 22:46
@VadimZhestikov VadimZhestikov requested a review from xeioex March 3, 2025 17:51
@xeioex
Copy link
Contributor

xeioex commented Mar 27, 2025

Hi @VadimZhestikov,

I will not comment on exact lines as there are too many changes.

In no particular order:

  1. we do not need two atom tables, njs_atom_map.h can be removed as redundant
  2. njs_flathsh_obj_t is the same as njs_flathsh_t with new special implementation for njs_flathsh_find() and njs_flathsh_insert().
  3. in lexer atom_id can fully replace unique_id
  4. vm->atom_hash_mem_pool and vm->atom_hash_atom_id_cell can be removed as well.

In addition, in order to improve the current patch the following function can be improved with direct uint32_t atom_id argument instead of njs_value_t *key: njs_property_query(), njs_value_property(), njs_value_property_set(), njs_value_property_delete()

See all the proposed improvements here:

"Improved atom_id use." should be applied on top of "Use atom_ids in the code." and eventually merged with it.
Two more improvements on top are: "Introduced NJS_VMCODE_PROPERTY_ATOM_SET instruction.", "Introduced NJS_VMCODE_PROPERTY_ATOM_GET instruction."

Benchmarks on my arm64:

MASTER
Richards: 603
Crypto: 1386
RayTrace: 618
NavierStokes: 2009
----
Score (version 7): 1009

VADIM INLINE
Richards: 971
Crypto: 1514
RayTrace: 674
NavierStokes: 2387
----
Score (version 7): 1240

XEIOEX
Richards: 984
Crypto: 1525
RayTrace: 804
NavierStokes: 2362
----
Score (version 7): 1299

@VadimZhestikov
Copy link
Contributor Author

Good changes, It will take me few days to review them and update all together.

@VadimZhestikov
Copy link
Contributor Author

VadimZhestikov commented Apr 11, 2025

Just intermediate changes (to be squashed later)

@xeioex
Copy link
Contributor

xeioex commented Apr 22, 2025

@VadimZhestikov

Some small patches on top

From 7a4e97ae689fdcff5dc779edc8b208e45b0807a4 Mon Sep 17 00:00:00 2001
From: Dmitry Volyntsev <[email protected]>
Date: Mon, 21 Apr 2025 17:06:33 -0700
Subject: [PATCH] Fixed DefineOwnProperty/key-is-not-numeric-index-throws v3

---
 src/njs_object.h         | 8 +++-----
 src/njs_object_prop.c    | 2 +-
 src/test/njs_unit_test.c | 5 +++++
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/njs_object.h b/src/njs_object.h
index 7bc03a9e6..4ece7e631 100644
--- a/src/njs_object.h
+++ b/src/njs_object.h
@@ -19,7 +19,7 @@ typedef enum {
     NJS_OBJECT_PROP_CONFIGURABLE = 16,
     NJS_OBJECT_PROP_WRITABLE = 32,
     NJS_OBJECT_PROP_UNSET = 64,
-    NJS_OBJECT_PROP_NOT_STRING = 128,
+    NJS_OBJECT_PROP_IS_STRING = 128,
 #define NJS_OBJECT_PROP_VALUE_ECW (NJS_OBJECT_PROP_VALUE                     \
                                    | NJS_OBJECT_PROP_ENUMERABLE              \
                                    | NJS_OBJECT_PROP_CONFIGURABLE            \
@@ -269,10 +269,8 @@ njs_object_prop_define_val(njs_vm_t *vm, njs_value_t *object, njs_value_t *name,
         }
     }
 
-    if (!njs_is_string(name)) {
-       flags |= NJS_OBJECT_PROP_NOT_STRING;
-    } else {
-       flags &= ~NJS_OBJECT_PROP_NOT_STRING;
+    if (njs_is_string(name)) {
+       flags |= NJS_OBJECT_PROP_IS_STRING;
     }
 
     return njs_object_prop_define(vm, object, name->atom_id, value, flags);
diff --git a/src/njs_object_prop.c b/src/njs_object_prop.c
index b72152727..4b9a1ec88 100644
--- a/src/njs_object_prop.c
+++ b/src/njs_object_prop.c
@@ -256,7 +256,7 @@ njs_object_prop_define(njs_vm_t *vm, njs_value_t *object, unsigned atom_id,
         }
 
         if (njs_slow_path(njs_is_typed_array(object) &&
-           !(flags & NJS_OBJECT_PROP_NOT_STRING)))
+           (flags & NJS_OBJECT_PROP_IS_STRING)))
         {
             /* Integer-Indexed Exotic Objects [[DefineOwnProperty]]. */
 
diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index 5ee222d6b..b8155ac5a 100644
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -5880,6 +5880,11 @@ static njs_unit_test_t  njs_test[] =
               "           return njs.dump(a) === `${v.name} [1,1,1]`})"),
       njs_str("true") },
 
+    { njs_str(NJS_TYPED_ARRAY_LIST
+              ".every(v=>{var a = new v([0]); var desc = Object.getOwnPropertyDescriptor(a, '0');"
+              "           try { Object.defineProperty(a, '1', desc) } catch (e) { return e.name == 'TypeError' }})"),
+      njs_str("true") },
+
     { njs_str(NJS_TYPED_ARRAY_LIST
               ".every(v=>{try {var a = new v([1,1]); Object.defineProperty(a, '1', {configurable:true})} "
               "           catch (e) { return e.message == 'Cannot redefine property: \"1\"'}})"),
From 923881004ea2ebd6566055c1a370ea3568132775 Mon Sep 17 00:00:00 2001
From: Dmitry Volyntsev <[email protected]>
Date: Mon, 21 Apr 2025 17:15:09 -0700
Subject: [PATCH] Added test for built-ins/Array/S15.4_A1.1_T10

---
 src/test/njs_unit_test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index b8155ac5a..13c9e00c6 100644
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -4172,6 +4172,11 @@ static njs_unit_test_t  njs_test[] =
     { njs_str("var a = [ 1, 2, 3 ]; a[4294967296] = 4; a + a[4294967296]"),
       njs_str("1,2,34") },
 
+    { njs_str("var x = []; var k = 1;"
+              "for (var i = 0; i < 32; i++) { k = k * 2; x[k - 2] = k; };"
+              "k = 1; for (i = 0; i < 32; i++) { k = k * 2; if (x[k - 2] != k) { throw 'error'; } }"),
+      njs_str("undefined") },
+
     { njs_str("delete[]['4e9']"),
       njs_str("true") },
 

I suggest the following resulting patches

Main patch:

Improved atom_id use.

Use atom_ids in the code.

 Fixed built-ins/TypedArrayConstructors/internals/DefineOwnProperty/key-is-not-numeric-index-throws v.2

Fixed built-ins/TypedArrayConstructors/internals/DefineOwnProperty/key-is-not-numeric-index-throws.

Fixed built-ins/Array/S15.4_A1.1_T10.

Separated namespaces for predefined STRING and SYMBOL names.

Fixed DefineOwnProperty/key-is-not-numeric-index-throws v3

Added test for built-ins/Array/S15.4_A1.1_T10

ATOM_GET patch:

Introduced NJS_VMCODE_PROPERTY_ATOM_GET instruction.

Put ATOM_GET changes from "Used more NJS_VMCODE_PROPERTY_ATOM_[SET|GET] instructions."

ATOM_SET patch:

Introduced NJS_VMCODE_PROPERTY_ATOM_SET instruction.

Put ATOM_SET changes from "Used more NJS_VMCODE_PROPERTY_ATOM_[SET|GET] instructions."

After the squash we will do the last cleanup/polish round with the main patch.

@VadimZhestikov
Copy link
Contributor Author

It was just a test before big squash.

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