Closed Bug 610198 Opened 9 years ago Closed 9 years ago

Replacing JS_GetStringBytes usage with JS_EncodeString

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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.
Attached patch v1Splinter Review
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?
Assignee: general → igor
Attachment #488738 - Flags: review?(mrbkap)
Attachment #488738 - Flags: review?(mrbkap) → review?(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.
Attachment #488738 - Flags: review?(gal) → review+
(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?
CString sounds good. JSAutoCString doesn't sound too bad. Brendan, any opinion? Its a public API.
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
http://hg.mozilla.org/tracemonkey/rev/8e119f847f97
Whiteboard: fixed-in-tracemonkey
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
A bunch of builds busted so I backed this out, sorry :( http://hg.mozilla.org/tracemonkey/rev/8e119f847f97
Whiteboard: fixed-in-tracemonkey
Blocks: 611428
(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.
(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.
http://hg.mozilla.org/tracemonkey/rev/d08fc0e5730e - relanded with compilation fixes in trace malloc debug code.
Whiteboard: fixed-in-tracemonkey
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 ?
Depends on: 613127
http://hg.mozilla.org/mozilla-central/rev/8e119f847f97
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.