Need API to store private data on JSExternalStrings

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sdwilsh, Assigned: luke)

Tracking

({dev-doc-complete})

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

8 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

8 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

8 years ago
Shawn: would this work for you?
Assignee: general → luke
Status: NEW → ASSIGNED
Reporter

Comment 3

8 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

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

Updated

8 years ago
Whiteboard: [spidernode]
Reporter

Comment 5

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

Comment 6

8 years ago
Posted 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)
Reporter

Comment 7

8 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

8 years ago
That is doable.  OOC, how does this come up?
Assignee

Comment 9

8 years ago
Posted patch now with JS_IsExternalString (obsolete) — Splinter Review
This work?
Attachment #527197 - Attachment is obsolete: true
Attachment #527197 - Flags: review?(nnethercote)
Reporter

Comment 10

8 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

8 years ago
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

8 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

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

Updated

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

Comment 17

8 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

Comment 19

8 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

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