707125 - Shrink the final string in str_unescape if possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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]]

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)]

OOC, is this a candidate for use of StringBuffer with an up front reservation? That already has extractWellSized functionality.

Comment 3

[Jeff Walden [:Waldo]]

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)]

(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]]

Er, right, you could do that.  I am not thinking clearly today.  :-)

Comment 6

[Nicholas Nethercote [inactive]]

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)]

(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)]

https://hg.mozilla.org/mozilla-central/rev/2c4639bc02fe

Comment 9

[Jeff Walden [:Waldo]]

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 [inactive]]

> 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.