610198 - Replacing JS_GetStringBytes usage with JS_EncodeString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

To minimize the patch for bug 607292 I file this bug to focus on replacing the JS_GetStringBytes usage with a helper class based on JS_EncodeString.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds JSAutoBytes class build on top of JS_EncodeString and switches most of the code to use the new class instead of JS_GetStringBytes/JS_GetStringBytesZ.

With the patch the only places that continue to depend on JS_GetStringBytes are the debugger code using it to report to the caller function name and JSData2Native/JSStringWithSize2Native from js/src/xpconnect/src/xpcconvert.cpp. Those cases would need some non-trivial refactoring that I pland to address in the bug 607292.

Blake, do you have to time to review this?

Comment 2

[Andreas Gal :gal]

Comment on attachment 488738 [details] [diff] [review]
v1

I think "JSAutoBytes" is a terrible name. How about JSAsciiString? Something that says that its a string. "Auto" in the name doesn't say much. Its totally overused. And thanks for doing this work.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> I think "JSAutoBytes" is a terrible name. How about JSAsciiString? 

The strings are not Ascii here.

> "Auto" in the name doesn't say much. Its totally overused.

Auto means a class that should not be allocated on the heap in general. This is exactly what is the suggested pattern here. It is pointless to allocate the class instances on the heap as the code can just pass the underlining C pointer. In that regard the class is similar to std::auto_ptr except it uses js_free, not delete, in its destructor.

So does JSAutoCString sound better?

Comment 4

[Andreas Gal :gal]

CString sounds good. JSAutoCString doesn't sound too bad. Brendan, any opinion? Its a public API.

Comment 5

[Brendan Eich [:brendan]]

JSAutoBytes should be named JSAutoCString or JSAutoByteString.

Auto is required, it's not a non-auto, it is RAII only, which must be explicitly named by a tempvar. JS_GUARD_OBJECT_NOTIFIER_PARAM is a giveaway.

XPCOM's string name is nsCAutoString, which misplaces the C IMHO, but we don't have to match.

Since the C char[] string encoding may not match this encoding, JSAutoByteString may be the better name. JSAutoEncodedByteString is too long and has deadwood.

/be

Comment 6

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/8e119f847f97

Comment 7

[Brendan Eich [:brendan]]

Often it wins to have a small char array as a member of a class such as JSAutoByteString -- do we have stats on distribution of string lengths that are deflated/decoded this way? It could be most are short enough to fit in a buffer we can tolerate allocating inline (we could tolerate a pretty big one).

Followup fodder but I wanted to mention it. Igor, what do you think?

/be

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

A bunch of builds busted so I backed this out, sorry :( http://hg.mozilla.org/tracemonkey/rev/8e119f847f97

Comment 9

[Igor Bukanov]

(In reply to comment #7)
> Often it wins to have a small char array as a member of a class such as
> JSAutoByteString -- do we have stats on distribution of string lengths that are
> deflated/decoded this way?

I do not have such stats. But pretty much all string conversions happens on slow paths (like error reporting). So until we would have stats about some slowdown caused by malloc/free I would prefer to save on code simplicity and size.

> Followup fodder but I wanted to mention it.

Filed as bug 611428.

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> A bunch of builds busted so I backed this out, sorry :(
> http://hg.mozilla.org/tracemonkey/rev/8e119f847f97

Sorry about the bust - I should not assume that a try server green for the whole patch queue means green for a particular patch.

Comment 11

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/d08fc0e5730e - relanded with compilation fixes in trace malloc debug code.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Are there any web-perf-relevant cases that hit the |string| case in qsgen.py (I sorta hope not, actually), and how does the perf of the new code compare to the old if there are?

Similarly, are there web-perf-relevant cases hitting xpc_qsJsvalToCharStr ?

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8e119f847f97