-
Notifications
You must be signed in to change notification settings - Fork 899
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
Add WeakRef Implementation #337
base: master
Are you sure you want to change the base?
Conversation
c8fd041
to
3e8d0d6
Compare
@saghul Thanks for the info. This PR focused more on generic implementation. Instead of using Also the |
Based on the PR, I am doing other commit. Since each of them has a fairly considerable amount of code, I'm going to separate them into three PRs. Looking forward to the code review, and I'd like to fix any comment ASAP ;) Follow ups:
|
I have not reviewed it, I was just pointing out that there is prior art. We do have the intention to merge both projects but at this stage I wouldn't necessarily pick a new implementation but take the existing one and improve where necessary. Just my 2 cents. |
Note that I did not merge the quickjs-ng WeakRef implementation because it adds a pointer in each JSString which adds too much memory overhead IMHO. This implementation is not better on this topic. Ideally, the "first_weak_ref" pointer in JSObject should also be removed. My idea was to add a bit in JSString/JSObject telling that they may be pointed by a weak reference. Another structure (a hash table) would be added to find the weak reference when the JSString or JSObject is freed. With this solution, the added memory is proportional to the number of values pointed by weak references instead of being proportional to the number of JSObject/JSString. There is a small added cost in case a JSObject/JSString pointed by a weak reference is freed, but I doubt it is critical. |
@bellard Thanks for the reply. I understand your thoughts and did notice the TODO item in the project.
I'd like to merge the commit into this PR.
QuestionsBefore that commit merges into this PR, there are two questions to be determined. 1. What does this comment mean, at the bottom of /* byte sizes: 40/48/72 */ By my guess, the But what does the 2. A generic hash-map implementation is added, which takes around 200 lines of code. So a new one is added, but it is similar to But it's up to the maintainers' decision, and I'd like to commit changes based on that. |
279034d
to
8fdcfba
Compare
1. Refactor JSObject weak reference list to support different kind of weak reference 2. Support Symbol as WeakMap key
JSValue object_data; /* for JS_SetObjectData(): 8/16/16 bytes */ | ||
} u; | ||
/* byte sizes: 40/48/72 */ | ||
/* byte sizes: 40/48/72 */ // TODO: update?, what is 48??? 40 is size on 32-bit platform, 64 72 on 64-bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion: #337 (comment)
1. Add a generic hashmap implementation. 2. Change Error `JSObject.is_uncatchable_error` flag to `JSObject.u.error` struct, leaving room for weak ref flag bit. 3. Compat `JSString.hash_next` to 31-bits (can be safely compat to 28-bits), leaving room for weak ref flag bit. Save memory for each JSObject (8 bytes on 64-bit platform, 4 bytes on 32-bit platform).
This PR did several related things.
1. Refactor JSObject weak reference list to support different kinds of weak reference
Changed
JSObject.u.first_weak_ref
, which was designed for WeakMap/WeakSet only. Also added weak reference support toJSString
(ie. Atom/Symbol ).It makes it easy to add
WeakRef
implementation. (And possiblyFinalizationRegistry
which is almost done, will create another PR after this one ).2. Add WeakRef implementation
Added standard WeakRef implementation together with test cases.
Referenced to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef
3. Support Symbol as WeakMap key
As a nice-to-have thing, also closed a TODO item in the doc.
quickjs/doc/quickjs.texi
Line 286 in 6e2e68f
4. Change JSObject.weak_ref_list to use a bit and external hash table.
Save memory for each JSObject (8 bytes on 64-bit platform, 4 bytes on 32-bit platform), while still maintaining good performance.
Performance test result
Test Code
Test Result:
(tested on Macbook Pro with M2 Pro chip, with 1 million iterations, time-unit is millisecond)
JSObject.first_weakref
pointerJSObject.has_weak_ref
and aJSRuntime.weak_target_map
Furthermore, theoretically, Objects/Symbols that are not weakly referenced have no performance impact.
Also added a generic (good performance) hash map implementation.
I tried to refactor JS Map based on that which resulted in a good performance improvement (LanderlYoung#4).
Follow ups
other commits based on this PR:
Ready to open other PRs once this PR is merged.