Last Comment Bug 707125 - Shrink the final string in str_unescape if possible
: Shrink the final string in str_unescape if possible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-02 03:38 PST by Nicholas Nethercote [:njn]
Modified: 2011-12-05 18:09 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.31 KB, patch)
2011-12-02 03:38 PST, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-12-02 03:38:31 PST
Created attachment 578540 [details] [diff] [review]
patch

str_unescape() currently allocates a new string with the same length as the input string, but it might not need all that space once the escapes are removed.  The excess chars can trigger an assertion in the "string-chars" memory reporter's sanity checking of |usable| and |computedSize|.

This patch changes str_unescape() so it reallocs the string if any escapes were removed.  The other possibility was to scan the string first to count the escapes, and only alloc once;  this function isn't called much, so it didn't seem like it mattered which approach I took, and realloc'ing was simpler.
Comment 1 Luke Wagner [:luke] 2011-12-02 08:12:04 PST
Comment on attachment 578540 [details] [diff] [review]
patch

>+    if (escapeFound) {
>+        JS_ASSERT(ni < length);
>+        jschar *tmpchars = (jschar *) cx->realloc_(newchars, (ni + 1) * sizeof(jschar));

Is jemalloc's realloc when (length - ni) is small really-fast?  If not, could you have

  if (escapeFound && (length - ni) >= (length / 4))

or something?
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-12-02 11:23:57 PST
OOC, is this a candidate for use of StringBuffer with an up front reservation? That already has extractWellSized functionality.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-02 13:57:42 PST
Yeah, I'd think StringBuffer would be the best thing to use here.  More patch to write to do it, tho, and maybe more work to do more reallocs and stuff, but this is hardly perf-sensitive stuff.
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-12-02 14:26:15 PST
(In reply to Jeff Walden (remove +bmo to email) from comment #3)
> Yeah, I'd think StringBuffer would be the best thing to use here.  More
> patch to write to do it, tho, and maybe more work to do more reallocs and
> stuff, but this is hardly perf-sensitive stuff.

Why more reallocs? Can't you just reserve up front (the maximum size) and then shrink it to size when you're done?
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-02 14:39:32 PST
Er, right, you could do that.  I am not thinking clearly today.  :-)
Comment 6 Nicholas Nethercote [:njn] 2011-12-04 21:02:16 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4639bc02fe

I ended up not using StringBuffer because I didn't like that I had to look through the implementations of multiple methods within both StringBuffer and Vector to convince myself that it actually did what I wanted.  The more direct route seemed better.
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2011-12-05 09:29:56 PST
(In reply to Nicholas Nethercote [:njn] from comment #6)
> I didn't like that I had to look
> through the implementations of multiple methods within both StringBuffer and
> Vector to convince myself that it actually did what I wanted

How can we fix that going forward? I thought reading the implementation (and the recommendation of peers) would be standard practice for using a suitable API.
Comment 8 Matt Brubeck (:mbrubeck) 2011-12-05 10:33:37 PST
https://hg.mozilla.org/mozilla-central/rev/2c4639bc02fe
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-05 12:55:41 PST
I would think StringBuffer functionality is a pretty common idiom, such that it wouldn't really need explanation beyond what you'd get from just looking at its definition in the header.  What about it made this not the case?

...so yeah, comment 7 +1.  :-)
Comment 10 Nicholas Nethercote [:njn] 2011-12-05 15:03:59 PST
> I thought reading the implementation (and
> the recommendation of peers) would be standard practice for using a suitable
> API.

Reading the interface, yes.  Reading the implementation, no, especially when I had to read the implementation of multiple functions in both StringBuffer and Vector and I still wasn't 100% certain it did what I wanted. 

(In reply to Jeff Walden (remove +bmo to email) from comment #9)
> I would think StringBuffer functionality is a pretty common idiom, such that
> it wouldn't really need explanation beyond what you'd get from just looking
> at its definition in the header.  What about it made this not the case?

StringBuffer is poorly documented, and I'm not going to assume anything about the low-level memory allocation behaviour of any code based on idioms.

> How can we fix that going forward?

Better documentation for StringBuffer would be a good start.

Note You need to log in before you can comment on or make changes to this bug.