724810 - Remove JS_AddExternalStringFinalizer and store the external string finalizer directly in string

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: 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.

Comment 2

[Igor Bukanov]

Attachment: 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.

Comment 3

[Luke Wagner [:luke]]

Righteous.  This has always bugged me.

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

Comment 4

[Igor Bukanov]

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

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83e8e93d85f8

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

[Luke Wagner [:luke]]

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).

Comment 9

[Eddy Bruel [:ejpbruel]]

(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

[Luke Wagner [:luke]]

Sweet!  Olli already did: bug 776000.