916993 - GC: Handlify public string methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Terrence Cole [:terrence]]

Attachment: handlify_string_apis-wip0.diff

I had to add a bunch of new Rooted in paths that look like they may be hot in order to support a JS_InternJSString that takes a Handle. All but one use was in a tail position, and thus did not need to be rooted here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805743 [details] [diff] [review]
handlify_string_apis-wip0.diff

Looks fine to me.  None of these are that hot; worker events are the closest, but still not that hot.

Comment 3

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=495c3f75cf07

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805743 [details] [diff] [review]
handlify_string_apis-wip0.diff

r=me on the non-jsapi.h bits.  That part could use a JS peer, though seems fine to me.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 805743 [details] [diff] [review]
handlify_string_apis-wip0.diff

Thanks, Boris!

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 805743 [details] [diff] [review]
handlify_string_apis-wip0.diff

Review of attachment 805743 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but did we decide to use the JS::HandleValue style rather than JS::Hande<JS::Value> in in jsapi.h now?

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef7f6e82c16


(In reply to Jon Coppeard (:jonco) from comment #6)
> Comment on attachment 805743 [details] [diff] [review]
> handlify_string_apis-wip0.diff
> 
> Review of attachment 805743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but did we decide to use the JS::HandleValue style rather than
> JS::Hande<JS::Value> in in jsapi.h now?

Yes, there was a long thread on [JS-internals] titled "What's in a (handle's) name?". I consider Luke's arguments in [1] to have overwhelmingly "won" the discussion in the favor of typedefs.

1 - http://www.mail-archive.com/[e-mail]/msg00415.html

Comment 8

[Terrence Cole [:terrence]]

After discussion on IRC last night, we have decided to handlify all the things, even methods which do not necessarily GC. I'm going to upload a second patch which handles the rest of the string methods (except for JS_GetStringLength, which Tom is working on).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ef7f6e82c16

Comment 10

[Tom S [:evilpie]]

What is the status here?

Comment 11

[Terrence Cole [:terrence]]

I think we got everything that can GC. I don't think we want to force rooting for things that cannot GC.

Comment 12

[Florian Scholz (Open Web Docs)]

Doc triaging:

Done (thanks :arai):
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_InternJSString
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewDependentString
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_ConcatStrings

Update needed:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_ParseJSON

New page needed:
https://dxr.mozilla.org/mozilla-central/search?q=JS_Stringify