Closed
Bug 610198
Opened 14 years ago
Closed 14 years ago
Replacing JS_GetStringBytes usage with JS_EncodeString
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
185.43 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #488738 -
Flags: review?(mrbkap) → review?(gal)
Comment 2•14 years ago
|
||
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+
Assignee | ||
Comment 3•14 years ago
|
||
(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•14 years ago
|
||
CString sounds good. JSAutoCString doesn't sound too bad. Brendan, any opinion? Its a public API.
Comment 5•14 years ago
|
||
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
Assignee | ||
Comment 6•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/8e119f847f97
Whiteboard: fixed-in-tracemonkey
Comment 7•14 years ago
|
||
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
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Assignee | ||
Comment 11•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/d08fc0e5730e - relanded with compilation fixes in trace malloc debug code.
Whiteboard: fixed-in-tracemonkey
Comment 12•14 years ago
|
||
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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8e119f847f97
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•