Closed
Bug 1063257
Opened 10 years ago
Closed 10 years ago
Implement JS::ubi::Node::size for JSString referents
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jimb, Assigned: jimb)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
12.63 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
WIP, no tests yet.
Assignee | ||
Comment 2•10 years ago
|
||
Okay, this has some pathetic tests. Please suggest more; Jason offered to do so as well.
Attachment #8484659 -
Attachment is obsolete: true
Attachment #8488900 -
Flags: review?(sphink)
Attachment #8488900 -
Flags: feedback?(jorendorff)
Comment 3•10 years ago
|
||
You could also assert the size of a TwoByte string > the size of a Latin1 string.
Comment 4•10 years ago
|
||
Comment on attachment 8488900 [details] [diff] [review] Implement JS::ubi::Node::size for JSString. Review of attachment 8488900 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/heap-analysis/byteSize-of-string.js @@ +35,5 @@ > +assertEq(ropeSize < flattenedRopeSize, true); > + > +// Surely we can assert these are true. > +assertEq(flattenedRopeSize > ropeSize, true); > +assertEq(flattenedRopeSize > 10000, true); This seems pretty weak too, but sizeof(str) > str.length? Or will that even hold if we share string data? Argh. (We might even do lazy dedup.)
Attachment #8488900 -
Flags: review?(sphink) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8488900 [details] [diff] [review] Implement JS::ubi::Node::size for JSString. Review of attachment 8488900 [details] [diff] [review]: ----------------------------------------------------------------- Why not assert hard bounds on the sizes of these things? If byteSize("blurf") is 16, why not assert that it's <= 16? It's *good* to have tests for that. Future people working on the String representation *should* have to change some tests when they regress memory usage. You can assert that large .slice()s of a large string do not take up much memory (i.e. .slice() returns dependent strings). Same goes for the strings in a RegExp#match result. It might be worth testing extensible strings (a special case of flattening that reduces total work across a particular serial-flattening pattern, documented by an excellent comment in flattenInternal).
Attachment #8488900 -
Flags: feedback?(jorendorff) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #5) > Comment on attachment 8488900 [details] [diff] [review] > Implement JS::ubi::Node::size for JSString. > > Review of attachment 8488900 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why not assert hard bounds on the sizes of these things? If > byteSize("blurf") is 16, why not assert that it's <= 16? It's *good* to have > tests for that. Future people working on the String representation *should* > have to change some tests when they regress memory usage. > > You can assert that large .slice()s of a large string do not take up much > memory (i.e. .slice() returns dependent strings). Same goes for the strings > in a RegExp#match result. > > It might be worth testing extensible strings (a special case of flattening > that reduces total work across a particular serial-flattening pattern, > documented by an excellent comment in flattenInternal). Excellent. I shall implement all of these suggestions.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > You could also assert the size of a TwoByte string > the size of a Latin1 > string. Okay.
Assignee | ||
Comment 8•10 years ago
|
||
Okay, here's a new version of the patch with completely revamped tests.
Attachment #8488900 -
Attachment is obsolete: true
Attachment #8576455 -
Flags: review?(sphink)
Assignee | ||
Comment 9•10 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa38e148859
Comment 10•10 years ago
|
||
Comment on attachment 8576455 [details] [diff] [review] Implement JS::ubi::Node::size for JSString. Review of attachment 8576455 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with this as-is. ::: js/src/jit-test/tests/heap-analysis/byteSize-of-string.js @@ +37,5 @@ > +// > +// 32-bit 64-bit test > +// representation Latin-1 char16_t Latin-1 char16_t label > +// ======================================================================== > +// JSExternalString (cannot be tested in shell) - That's sad. That would be very useful for fuzzing them. This is mean, but can you file a bug for that? @@ +54,5 @@ > +assertEq(tByteSize("123456789.123456789.123"), s(32, 32)); // F, F > +assertEq(tByteSize("123456789.123456789.1234"), s(48, 56)); // X, X > +assertEq(tByteSize("123456789.123456789.123456789.1"), s(48, 56)); // X, X > +assertEq(tByteSize("123456789.123456789.123456789.12"), s(64, 72)); // X, X > + Heh. Nice length indicators! @@ +60,5 @@ > +// "Impassionate gods have never seen the red that is the Tatsuta River." > +// - Ariwara no Narihira > +assertEq(tByteSize("千"), s(16, 24)); // T, T > +assertEq(tByteSize("千早"), s(16, 24)); // T, T > +assertEq(tByteSize("千早ぶ"), s(16, 24)); // T, T Is the indentation just messed up in splinter, or are there 3 lines that don't match up? @@ +92,5 @@ > +// > +// Flatting that should turn the original rope into a dependent string, and > +// yield a new linear string, of the some size as the original. > +rope8a = rope8 + fragment8; > +assertEq(tByteSize(rope8a), s(16, 24)); Wait, so I wasn't paying attention. Is there a way to also look at the size of the other pieces of the rope, or the whole thing? Or is that regarded as too implementation-unstable?
Attachment #8576455 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for the review! (In reply to Steve Fink [:sfink, :s:] from comment #10) > > +// representation Latin-1 char16_t Latin-1 char16_t label > > +// ======================================================================== > > +// JSExternalString (cannot be tested in shell) - > > That's sad. That would be very useful for fuzzing them. This is mean, but > can you file a bug for that? You're so mean. I tell people all the time, "That Steve Fink? He's *mean*." Filed as bug 1145429. > @@ +60,5 @@ > > +// "Impassionate gods have never seen the red that is the Tatsuta River." > > +// - Ariwara no Narihira > > +assertEq(tByteSize("千"), s(16, 24)); // T, T > > +assertEq(tByteSize("千早"), s(16, 24)); // T, T > > +assertEq(tByteSize("千早ぶ"), s(16, 24)); // T, T > > Is the indentation just messed up in splinter, or are there 3 lines that > don't match up? I think the Japanese characters don't quite occupy two character cells, so the tabs render differently depending on who's drawing the text. Not using tabs made every line ragged. > @@ +92,5 @@ > > +// > > +// Flatting that should turn the original rope into a dependent string, and > > +// yield a new linear string, of the some size as the original. > > +rope8a = rope8 + fragment8; > > +assertEq(tByteSize(rope8a), s(16, 24)); > > Wait, so I wasn't paying attention. Is there a way to also look at the size > of the other pieces of the rope, or the whole thing? Or is that regarded as > too implementation-unstable? Well, when you make a rope, its LHS and RHS are simply the operands to the concatenation. So you can store them in a variable and refer to them normally. Note that, immediately after the line you commented on, I test that the size of rope8 has changed from {16,24} + 65536 (as a JSExtensibleString) to just {16,24} (as a JSDependentString). Before that, I asserted that it changed from {16,24} (as a rope) to the extensible string.
Updated•10 years ago
|
Blocks: memory-platform
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite+
Target Milestone: --- → mozilla40
Assignee | ||
Comment 13•10 years ago
|
||
Here is a clean try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e827e9fa5d0
Comment 14•10 years ago
|
||
Backed out for jit-test failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9d047bd0d3 https://treeherder.mozilla.org/logviewer.html#?job_id=8951390&repo=mozilla-inbound
Assignee | ||
Comment 15•10 years ago
|
||
Okay, with the patch in bug 1155438, this is green on OSX 10.6 debug JIT tests.
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f65d5df4138b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•