The default bug view has changed. See this FAQ.

Need API to store private data on JSExternalStrings

RESOLVED FIXED in mozilla6

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sdwilsh, Assigned: luke)

Tracking

({dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spidernode][fixed-in-tracemonkey])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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*.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 526938 [details] [diff] [review]
add JS_NewExternalStringWithClosure

Shawn: would this work for you?
Assignee: general → luke
Status: NEW → ASSIGNED
(Reporter)

Comment 3

6 years ago
(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).
(Reporter)

Comment 4

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

Updated

6 years ago
Whiteboard: [spidernode]
(Reporter)

Comment 5

6 years ago
Luke: is there anything I can do to help get this landed?
(Assignee)

Comment 6

6 years ago
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 :)
Attachment #526938 - Attachment is obsolete: true
Attachment #527197 - Flags: review?(nnethercote)
(Reporter)

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
That is doable.  OOC, how does this come up?
(Assignee)

Comment 9

6 years ago
Created attachment 527305 [details] [diff] [review]
now with JS_IsExternalString

This work?
Attachment #527197 - Attachment is obsolete: true
Attachment #527197 - Flags: review?(nnethercote)
(Reporter)

Comment 10

6 years ago
(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!
(Assignee)

Comment 11

6 years ago
Created attachment 527336 [details] [diff] [review]
now with CHECK_REQUEST

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+
(Assignee)

Comment 13

6 years ago
(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
(Assignee)

Comment 14

6 years ago
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]
http://hg.mozilla.org/mozilla-central/rev/905a35f3a76b
http://hg.mozilla.org/mozilla-central/rev/15df44504237
http://hg.mozilla.org/mozilla-central/rev/d5fcfa622f16
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Keywords: dev-doc-needed
I assume this is for Mozilla 6; it's not tagged as such though.
(Reporter)

Comment 17

6 years ago
(In reply to comment #16)
> I assume this is for Mozilla 6; it's not tagged as such though.
Correct.
Target Milestone: --- → mozilla6
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
(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.