Closed Bug 651041 Opened 10 years ago Closed 10 years ago

Need API to store private data on JSExternalStrings

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: sdwilsh, Assigned: luke)

Details

(Keywords: dev-doc-complete, Whiteboard: [spidernode][fixed-in-tracemonkey])

Attachments

(1 file, 3 obsolete files)

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*.
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.
Shawn: would this work for you?
Assignee: general → luke
Status: NEW → ASSIGNED
(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).
(In reply to comment #2)
> Shawn: would this work for you?
Yes, I think that'd be perfect!
Whiteboard: [spidernode]
Luke: is there anything I can do to help get this landed?
Attached patch now with test, touchups (obsolete) — Splinter Review
I'll take care of it.  I just had to write a jsapi-test first :)
Attachment #526938 - Attachment is obsolete: true
Attachment #527197 - Flags: review?(nnethercote)
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
That is doable.  OOC, how does this come up?
Attached patch now with JS_IsExternalString (obsolete) — Splinter Review
This work?
Attachment #527197 - Attachment is obsolete: true
Attachment #527197 - Flags: review?(nnethercote)
(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!
Great!  Back to r?njn.
Attachment #527305 - Attachment is obsolete: true
Attachment #527336 - Flags: review?(nnethercote)
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.
Attachment #527336 - Flags: review?(nnethercote) → review+
(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
Conservative GC seems to be keeping strings alive in my jsapi-test, so I made it fuzzy:
http://hg.mozilla.org/tracemonkey/rev/d5fcfa622f16
Whiteboard: [spidernode] → [spidernode][fixed-in-tracemonkey]
Keywords: dev-doc-needed
I assume this is for Mozilla 6; it's not tagged as such though.
(In reply to comment #16)
> I assume this is for Mozilla 6; it's not tagged as such though.
Correct.
Target Milestone: --- → mozilla6
(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.
(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.
You need to log in before you can comment on or make changes to this bug.