Closed Bug 1063257 Opened 5 years ago Closed 5 years ago

Implement JS::ubi::Node::size for JSString referents

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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)

No description provided.
WIP, no tests yet.
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)
You could also assert the size of a TwoByte string > the size of a Latin1 string.
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 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+
(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.
(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.
Okay, here's a new version of the patch with completely revamped tests.
Attachment #8488900 - Attachment is obsolete: true
Attachment #8576455 - Flags: review?(sphink)
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+
Depends on: 1145429
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.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → mozilla40
Depends on: 1155438
Okay, with the patch in bug 1155438, this is green on OSX 10.6 debug JIT tests.
https://hg.mozilla.org/mozilla-central/rev/f65d5df4138b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.