The default bug view has changed. See this FAQ.

Shrink the final string in str_unescape if possible

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
Attachment #578540 - Flags: review?(luke)

Comment 1

5 years ago
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?
Attachment #578540 - Flags: review?(luke) → review+
OOC, is this a candidate for use of StringBuffer with an up front reservation? That already has extractWellSized functionality.
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.
(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?
Er, right, you could do that.  I am not thinking clearly today.  :-)
(Assignee)

Comment 6

5 years ago
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/2c4639bc02fe
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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.  :-)
(Assignee)

Comment 10

5 years ago
> 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.
Target Milestone: mozilla11 → ---
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.