drsm
All 24 comments
Here is the draft of the patch that adds only a primitive data type:
https://gist.github.com/drsm/1cf394eaf4cc2ff8f4ef50025bb687cd
drsm
on 15 Nov 2019
@drsm Good start!
I planned to implement this feature after #16.
BTW, we also need built-in tags (like to @@toStringTag).
xeioex
on 15 Nov 2019
@drsm
BTW, cannot we reduce the number of tests by combining?
Something like
["0**", "1/"].every((v)=> {try {new Function(v + "Symbol()")() } catch (e) {return e.message === "Cannot convert a Symbol value to a number"}})
@xeioex
BTW, cannot we reduce the number of tests by combining?
Done.
drsm
on 18 Nov 2019
@drsm
Can you share current test262 update?
xeioex
on 18 Nov 2019
@xeioex
=== Summary ===
- Ran 31860 tests
- - Passed 10031 tests (31.5%)
- - Failed 21829 tests (68.5%)
+ - Passed 10210 tests (32.0%)
+ - Failed 21650 tests (68.0%)
@drsm
Is it ready for review? What issues are left?
xeioex
on 19 Nov 2019
@xeioex
Is it ready for review?
Yes.
What issues are left?
Support for object property keys and Symbol.for()/.keyFor() are not a subject of the patch.
So, only wrong error message in Symbol() + 'abc'
drsm
on 19 Nov 2019
Also, I forget about
> JSON.stringify(Symbol())
undefined
> JSON.stringify({ a: Symbol(), b: 1 })
'{"b":1}'
>
But I think it's better to fix this after property keys.
drsm
on 19 Nov 2019
@drsm I see a lot of
TypeError: Cannot convert a Symbol value to a string
is it hard to implement?
xeioex
on 19 Nov 2019
@xeioex
is it hard to implement?
No. Just involves some weird logic, but on slow path.
Symbol() + 'abc' is fixed now.
drsm
on 19 Nov 2019
Is it ready for review?
Yes.
Some files were forgotten after rebase.
Sorry about that, fixed now.
drsm
on 19 Nov 2019
@drsm
Support for object property keys .. are not a subject of the patch.
Do you mean Something like var o = {}; o[Symbol.isConcatSpreadable] = true right? Do you plan to add this later?
xeioex
on 20 Nov 2019
@drsm
Great job! Will commit with small improvements over: https://gist.github.com/dd31240f02ed6c8edb1707c8f11aaf08
xeioex
on 20 Nov 2019
@xeioex
Thanks for reviewing this!
Do you mean Something like
var o = {}; o[Symbol.isConcatSpreadable] = trueright?
Yes. And also support for Object.defineProperty(), Object.getOwnPropertyDescriptor() and other related functions.
Do you plan to add this later?
Yes. I wil try.
drsm
on 20 Nov 2019
@drsm
I plan to add support for object property keys and "@@toStringTag" for builtin objects.
xeioex
on 20 Nov 2019
@xeioex
My plan was to abuse lhq->key value as
lhq->key = njs_str_t(njs_symbol_key(), NULL);
lhq->key_hash = njs_symbol_hash();
// https://stackoverflow.com/a/12996028
uint32_t njs_symbol_hash(uint32_t x) {
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = (x >> 16) ^ x;
return x;
}
I think we need something generic or union here, will be also useful for Map and Set implementation:
> var m = new Set([null, 1, Symbol(), 'asdf', {}, void 0]);
undefined
> [...m.keys()].map((x) => typeof x)
[ 'object', 'number', 'symbol', 'string', 'object', 'undefined' ]
Looks like there is no need for lhq->key at all in case of symbol lookup.
If we change key generation scheme to:
uint32_t njs_hash(uint32_t x) {
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = (x >> 16) ^ x;
return x;
}
uint32_t njs_unhash(uint32_t x) {
x = ((x >> 16) ^ x) * 0x119de1f3;
x = ((x >> 16) ^ x) * 0x119de1f3;
x = (x >> 16) ^ x;
return x;
}
// in constructor:
key = njs_hash(++vm->symbol_generator);
// toString, description (they are used mostly for debugging):
string_index = njs_unhash(key)
// precompute njs_hash for well-known symbols
@drsm
Looks like there is no need for lhq->key at all in case of symbol lookup.
yes. since symbols Id are uniq we can simple do
njs_object_hash_test()
prop = data;
name = &prop->name;
if (njs_slow_path(njs_is_symbol(name))) {
return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK
: NJS_DECLINED;
}
/* string. */
...
Even more, we can avoid memcmp() even for strings altogether if we can make them uniq and compare just pointers or some ids similar to symbol ids. (I plan to do this in the middle-term future)
See https://docs.python.org/3/library/sys.html#sys.intern .
xeioex
on 21 Nov 2019
@drsm Compare also: https://bellard.org/quickjs/quickjs.html#Atoms
xeioex
on 21 Nov 2019
@xeioex
-return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK
+return (njs_symbol_key(name) == lhq->key_hash) && !lhq->key.length ? NJS_OK
@drsm
Take a look: https://gist.github.com/fcb8b28ac5fc6163db86fed7a9029fa5
Yes. And also support for Object.defineProperty(), Object.getOwnPropertyDescriptor() and other related functions.
done. + also Object.getOwnPropertyNames(), Object.getOwnPropertySymbols(), Object.keys().
xeioex
on 21 Nov 2019
One moment about this place.
Can't we get a hash collision here while looking up a string property with same hash as one of the object's symbols?
drsm
on 21 Nov 2019
@xeioex
Sorry for that:
-return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK +return (njs_symbol_key(name) == lhq->key_hash) && !lhq->key.length ? NJS_OK
two tries were not enough... )
$ build/njs
interactive njs 0.3.8
v.<Tab> -> the properties and prototype methods of v.
>> var o = {}, n = 5381 /* NJS_DJB_HASH_INIT */;
undefined
>> while(n--) o[Symbol()] = 'test';
undefined
>> o['']
'test'
# HG changeset patch
# User Artem S. Povalyukhin <[email protected]>
# Date 1574364674 -10800
# Thu Nov 21 22:31:14 2019 +0300
# Node ID 48821a4feda31a5189c0490ef30c7183a91ef9fd
# Parent 10a19a2e1d4feca045ca98a271db6c71bfaeb9cc
Fixed hash collision for empty string key.
diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_object.c
--- a/src/njs_object.c Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_object.c Thu Nov 21 22:31:14 2019 +0300
@@ -171,7 +171,7 @@ njs_object_hash_test(njs_lvlhsh_query_t
if (njs_slow_path(njs_is_symbol(name))) {
return ((njs_symbol_key(name) == lhq->key_hash)
- && lhq->key.length == 0) ? NJS_OK : NJS_DECLINED;
+ && lhq->key.start == NULL) ? NJS_OK : NJS_DECLINED;
}
/* string. */
diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_object.h
--- a/src/njs_object.h Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_object.h Thu Nov 21 22:31:14 2019 +0300
@@ -110,6 +110,7 @@ njs_object_property_key_set(njs_lvlhsh_q
if (njs_is_symbol(key)) {
lhq->key.length = 0;
+ lhq->key.start = NULL;
lhq->key_hash = njs_symbol_key(key);
} else {
diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_value.h
--- a/src/njs_value.h Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_value.h Thu Nov 21 22:31:14 2019 +0300
@@ -871,6 +871,7 @@ njs_set_object_value(njs_value_t *value,
#define njs_property_query_init(pq, _query, _own) \
do { \
(pq)->lhq.key.length = 0; \
+ (pq)->lhq.key.start = NULL; \
(pq)->lhq.value = NULL; \
(pq)->own_whiteout = NULL; \
(pq)->query = _query; \
diff -r 10a19a2e1d4f -r 48821a4feda3 src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c Thu Nov 21 20:56:06 2019 +0300
+++ b/src/test/njs_unit_test.c Thu Nov 21 22:31:14 2019 +0300
@@ -10302,6 +10302,10 @@ static njs_unit_test_t njs_test[] =
"Object.getOwnPropertyDescriptor(o, Symbol.isConcatSpreadable).value"),
njs_str("true") },
+ { njs_str("var o = {}, n = 5381 /* NJS_DJB_HASH_INIT */;"
+ "while(n--) o[Symbol()] = 'test'; o[''];"),
+ njs_str("undefined") },
+
/* String */
{ njs_str("String()"),
Related issues
pavelsevcik
路
4Comments
xeioex
路
3Comments
an0ma1ia
路
4Comments
porunov
路
3Comments
drsm
路
4Comments