Last Comment Bug 651041 - Need API to store private data on JSExternalStrings
: Need API to store private data on JSExternalStrings
Status: RESOLVED FIXED
[spidernode][fixed-in-tracemonkey]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-18 23:09 PDT by Shawn Wilsher :sdwilsh
Modified: 2011-04-28 17:23 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add JS_NewExternalStringWithClosure (6.73 KB, patch)
2011-04-18 23:41 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
now with test, touchups (11.70 KB, patch)
2011-04-19 22:49 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
now with JS_IsExternalString (11.99 KB, patch)
2011-04-20 09:55 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
now with CHECK_REQUEST (12.01 KB, patch)
2011-04-20 11:48 PDT, Luke Wagner [:luke]
n.nethercote: review+
Details | Diff | Review

Description Shawn Wilsher :sdwilsh 2011-04-18 23:09:59 PDT
While trying to implement external strings in v8monkey, I've hit a bit of a snag.  The v8api gives us a pointer to an object that has the character data and a dispose method.  We clearly need to stash this somewhere so we can call Dispose when our finalizer is called.

I can work around this by using a hashtable to map JSString* to our ExternalAsciiStringResource*.
Comment 1 Luke Wagner [:luke] 2011-04-18 23:23:07 PDT
You are in luck!  A string header is 4 words and, for JSExternalString, 1 word is unused.

Also, do you mean ExternalStringResource?  It looks like ExternalAsciiStringResource holds ASCII chars which couldn't be used in SM directly without conversion.
Comment 2 Luke Wagner [:luke] 2011-04-18 23:41:13 PDT
Created attachment 526938 [details] [diff] [review]
add JS_NewExternalStringWithClosure

Shawn: would this work for you?
Comment 3 Shawn Wilsher :sdwilsh 2011-04-18 23:41:43 PDT
(In reply to comment #1)
> Also, do you mean ExternalStringResource?  It looks like
> ExternalAsciiStringResource holds ASCII chars which couldn't be used in SM
> directly without conversion.
Currently node only seems to use the Ascii version, so I'm focusing my efforts on that first.  With that being said, as long as we can store something on it, we can store what we need to store via a struct (and just free that struct when the object goes away).  We've had to do this with some things using JSObjects and just add a finalizer to clean up the struct (added with JS_SetPrivateData).
Comment 4 Shawn Wilsher :sdwilsh 2011-04-18 23:42:42 PDT
(In reply to comment #2)
> Shawn: would this work for you?
Yes, I think that'd be perfect!
Comment 5 Shawn Wilsher :sdwilsh 2011-04-19 20:29:53 PDT
Luke: is there anything I can do to help get this landed?
Comment 6 Luke Wagner [:luke] 2011-04-19 22:49:54 PDT
Created attachment 527197 [details] [diff] [review]
now with test, touchups

I'll take care of it.  I just had to write a jsapi-test first :)
Comment 7 Shawn Wilsher :sdwilsh 2011-04-20 07:24:39 PDT
I may have spoken too soon about this being enough.  There is one more thing I don't see an API for that we'd need:
- Testing if a JSString is a JSExternalString
Comment 8 Luke Wagner [:luke] 2011-04-20 09:17:20 PDT
That is doable.  OOC, how does this come up?
Comment 9 Luke Wagner [:luke] 2011-04-20 09:55:21 PDT
Created attachment 527305 [details] [diff] [review]
now with JS_IsExternalString

This work?
Comment 10 Shawn Wilsher :sdwilsh 2011-04-20 11:34:10 PDT
(In reply to comment #8)
> That is doable.  OOC, how does this come up?
The v8 API has an IsExternal() method on the String class, but we only store (and can only store) a jsval, so we have no way of answering that otherwise.

What you added works.  Thanks for the quick turnaround!
Comment 11 Luke Wagner [:luke] 2011-04-20 11:48:41 PDT
Created attachment 527336 [details] [diff] [review]
now with CHECK_REQUEST

Great!  Back to r?njn.
Comment 12 Nicholas Nethercote [:njn] 2011-04-20 16:28:22 PDT
Comment on attachment 527336 [details] [diff] [review]
now with CHECK_REQUEST

Looks fine except I want some comments.  Just from looking at the patch I
have no idea what the externalClosure can/should be used for.  I'd like:
- a brief mention of externalClosure's existence in the big comment above
  JSString, and 
- a brief description of what it can be used for in the comment above
  JS_NewExternalStringWithClosure.


>+/*
>+ * Return whether 'str' was created with JS_NewExternalString(WithClosure).
>+ */
>+extern JS_PUBLIC_API(JSBool)
>+JS_IsExternalString(JSContext *cx, JSString *str);

I don't understand that comment.  What does
"JS_NewExternalString(WithClosure)" mean?  Oh... is that Luke-speak for
saying that |WithClosure| is optional?  I'd prefer
|JS_NewExternalString{,WithClosure}| or just "JS_NewExternalString or
JS_NewExternalStringWithClosure" is probably best.
Comment 13 Luke Wagner [:luke] 2011-04-20 19:34:47 PDT
(In reply to comment #12)
> - a brief mention of externalClosure's existence in the big comment above
>   JSString, and 

The big JSString comment is more high-level and this is a nitty gritty detail, so I put the comment on JSExternalString (which is easily found since "JSExternalString" is written next to the field in the union).

http://hg.mozilla.org/tracemonkey/rev/905a35f3a76b
http://hg.mozilla.org/tracemonkey/rev/15df44504237
Comment 14 Luke Wagner [:luke] 2011-04-20 21:09:46 PDT
Conservative GC seems to be keeping strings alive in my jsapi-test, so I made it fuzzy:
http://hg.mozilla.org/tracemonkey/rev/d5fcfa622f16
Comment 16 Eric Shepherd [:sheppy] 2011-04-26 19:16:27 PDT
I assume this is for Mozilla 6; it's not tagged as such though.
Comment 17 Shawn Wilsher :sdwilsh 2011-04-26 21:08:13 PDT
(In reply to comment #16)
> I assume this is for Mozilla 6; it's not tagged as such though.
Correct.
Comment 19 Colin Walters 2011-04-28 08:24:02 PDT
(In reply to comment #14)
> Conservative GC seems to be keeping strings alive in my jsapi-test, so I made
> it fuzzy:
> http://hg.mozilla.org/tracemonkey/rev/d5fcfa622f16

Pretty ugly; maybe it'd be better to have a JSAPI call to the garbage collector to do a *precise* collection?  If the conservative GC is just to avoid lots of complex rooting calls over C code, it should still be safe to do a precise GC at certain points.  Though I could certainly imagine this tripping bugs in spidermonkey itself, so maybe it would be too big of a project.
Comment 20 Luke Wagner [:luke] 2011-04-28 17:23:22 PDT
(In reply to comment #18)
Thanks!

(In reply to comment #19)
I felt ugly writing it :)  Yeah, that's a good idea for an internal interface used by, e.g., JSAPI tests.

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