627632 - Avoid silly malloc/realloc pattern in StringBuffer::finishString()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

finishString() uses extractRawBuffer() on a vector.  It then reallocs (shrinks) the resulting buffer if it has empty space.  But it doesn't allow for the possibility that the jsvector has inline storage, in which case the extracted buffer will be malloc()'d to just the right size.

This patch fixes this.  It exposes usingInlineStorage(), which isn't great, but seems hard to avoid.

A simpler hack is to just increase sMinWasteSize to 100 or so, since inline storage is usually for smaller numbers of elements than that.  It's less exact though.

This avoids over 10,000 unnecessary reallocs in v8-regexp for a 1.003x instruction count win.

Comment 1

[Luke Wagner [:luke]]

(In reply to comment #0)

Cool observation.

> A simpler hack is to just increase sMinWasteSize to 100 or so, since inline
> storage is usually for smaller numbers of elements than that.  It's less exact
> though.

This would've been my first approach.  The static type of CharBuffer tells you exactly its inline storage so, to be exact and clear about your intentions in the code, you could expose

  static const size_t MaxInlineStorage = N;

in js::Vector and then just use CharBuffer::MaxInlineStorage directly in StringBuffer::finishString instead of sMinWasteSize.

Comment 2

[Nicholas Nethercote [inactive]]

Eh, this doesn't seem worth the effort.  If anyone disagrees, please feel to reassign to yourself :)

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Compare length against inline storage static.

Took a quick stab.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Compare length against inline storage static.

qref

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Compare length against inline storage static.

Man I suck at hg.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 522880 [details] [diff] [review]
Compare length against inline storage static.

The logic looks good.  I don't see any reason to tease out an extractWellSized() helper, though: it doesn't have any other callers and it doesn't seem to make sense as part of the StringBuffer interface. r+ with the extractWellSized function rolled into finishString or, if you'd like, an inline static helper.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/40d6f9795ecc

Talked to luke IRL and he agreed a private method helper makes sense here.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/40d6f9795ecc