651041 - Need API to store private data on JSExternalStrings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shawn Wilsher :sdwilsh]

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]]

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]]

Attachment: add JS_NewExternalStringWithClosure

Shawn: would this work for you?

Comment 3

[Shawn Wilsher :sdwilsh]

(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]

(In reply to comment #2)
> Shawn: would this work for you?
Yes, I think that'd be perfect!

Comment 5

[Shawn Wilsher :sdwilsh]

Luke: is there anything I can do to help get this landed?

Comment 6

[Luke Wagner [:luke]]

Attachment: now with test, touchups

I'll take care of it.  I just had to write a jsapi-test first :)

Comment 7

[Shawn Wilsher :sdwilsh]

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]]

That is doable.  OOC, how does this come up?

Comment 9

[Luke Wagner [:luke]]

Attachment: now with JS_IsExternalString

This work?

Comment 10

[Shawn Wilsher :sdwilsh]

(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]]

Attachment: now with CHECK_REQUEST

Great!  Back to r?njn.

Comment 12

[Nicholas Nethercote [inactive]]

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]]

(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]]

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 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/905a35f3a76b
http://hg.mozilla.org/mozilla-central/rev/15df44504237
http://hg.mozilla.org/mozilla-central/rev/d5fcfa622f16

Comment 16

[Eric Shepherd [:sheppy]]

I assume this is for Mozilla 6; it's not tagged as such though.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> I assume this is for Mozilla 6; it's not tagged as such though.
Correct.

Comment 18

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_NewExternalString
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetExternalStringClosure
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_IsExternalString

Also linked to from:

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference#Strings

Comment 19

[Colin Walters]

(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]]

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