Remove JS_AddExternalStringFinalizer and store the external string finalizer directly in string

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
25.42 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
With 4-word strings there is no need in JS_AddExternalStringFinalizer API as the finalizer can be stored directly in the string. This should allow to simplify external string implementation and usage.
(Assignee)

Comment 1

5 years ago
Created attachment 594964 [details] [diff] [review]
v1

The patch removes JS_(AddRemove)ExternalStringFinalizer and changes JS_NewExternalString to take the finalizer directly. I also changed the finalizer to be a struct holding a pointer to the finalization callback. This allowed to remove JS_NewExternalStringWithClosure as the embedding can just subclass JSStringFinalizer.

I also changed the signature of the callback to take char * pointer, not the original JSString. This is to facilitate the forthcoming patch to finalize the external strings in the background and to avoid the needless JS_GetStringCharsZ or similar calls in the finalizers.
Assignee: general → igor
Attachment #594964 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
Created attachment 594971 [details] [diff] [review]
v2

The patch removes the rt argument from the finalizer callback. It has no use in FF and, if necessary, an embedding can store any relevant pointers in the finalizer itself.
Attachment #594964 - Attachment is obsolete: true
Attachment #594964 - Flags: review?(luke)
Attachment #594971 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #594971 - Flags: review? → review?(luke)

Comment 3

5 years ago
Righteous.  This has always bugged me.

ejpbruel: I think this warrants and update to your blog post ;)

Updated

5 years ago
Attachment #594971 - Flags: review?(luke) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e8e93d85f8

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/83e8e93d85f8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
Depends on: 726964

Comment 6

5 years ago
So how can finalizer now get access to the closure data or some such.

Comment 7

5 years ago
The old API was more useful.
One could use static class/method as finalizer and pass some relevant data as closure.
Now one would need to create a new instance of the finalizer each time external string is created.
Not good if we want to share for example nsIAtom data as external strings in a fast way.

Comment 8

5 years ago
It looks like noone was really using the closure word when the bug was filed so Igor just removed it.  However, there is still space in the JSString header to store a closure word (String.d.s.u3) so if you think this is an important use case we can just put back JS_NewExternalStringWithClosure and add the closure parameter to the callback).
(In reply to Luke Wagner [:luke] from comment #8)
> It looks like noone was really using the closure word when the bug was filed
> so Igor just removed it.  However, there is still space in the JSString
> header to store a closure word (String.d.s.u3) so if you think this is an
> important use case we can just put back JS_NewExternalStringWithClosure and
> add the closure parameter to the callback).

If you open a bug for this I'm willing to take it. I should be able to make time for this next week.

Comment 10

5 years ago
Sweet!  Olli already did: bug 776000.

Updated

2 years ago
Depends on: 1115996
You need to log in before you can comment on or make changes to this bug.