Last Comment Bug 724810 - Remove JS_AddExternalStringFinalizer and store the external string finalizer directly in string
: Remove JS_AddExternalStringFinalizer and store the external string finalizer ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 726964 1115996
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-07 01:42 PST by Igor Bukanov
Modified: 2014-12-28 09:04 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (25.56 KB, patch)
2012-02-07 02:13 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (25.42 KB, patch)
2012-02-07 03:19 PST, Igor Bukanov
luke: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2012-02-07 01:42:09 PST
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 2012-02-07 02:13:16 PST
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.
Comment 2 Igor Bukanov 2012-02-07 03:19:26 PST
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.
Comment 3 Luke Wagner [:luke] 2012-02-07 08:06:29 PST
Righteous.  This has always bugged me.

ejpbruel: I think this warrants and update to your blog post ;)
Comment 5 Ed Morley [:emorley] 2012-02-08 09:40:22 PST
https://hg.mozilla.org/mozilla-central/rev/83e8e93d85f8
Comment 6 Olli Pettay [:smaug] 2012-07-20 07:40:00 PDT
So how can finalizer now get access to the closure data or some such.
Comment 7 Olli Pettay [:smaug] 2012-07-20 09:05:35 PDT
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] 2012-07-20 09:13:24 PDT
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] 2012-07-20 09:14:27 PDT
(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] 2012-07-20 09:15:54 PDT
Sweet!  Olli already did: bug 776000.

Note You need to log in before you can comment on or make changes to this bug.