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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jim Blandy :jimb]

Attachment: Implement JS::ubi::Node::size for JSString.

WIP, no tests yet.

Comment 2

[Jim Blandy :jimb]

Attachment: Implement JS::ubi::Node::size for JSString.

Okay, this has some pathetic tests. Please suggest more; Jason offered to do so as well.

Comment 3

[Jan de Mooij [:jandem]]

You could also assert the size of a TwoByte string > the size of a Latin1 string.

Comment 4

[Steve Fink [:sfink] [:s:]]

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

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[Jim Blandy :jimb]

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

Comment 7

[Jim Blandy :jimb]

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

Comment 8

[Jim Blandy :jimb]

Attachment: Implement JS::ubi::Node::size for JSString.

Okay, here's a new version of the patch with completely revamped tests.

Comment 9

[Jim Blandy :jimb]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa38e148859

Comment 10

[Steve Fink [:sfink] [:s:]]

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?

Comment 11

[Jim Blandy :jimb]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47abda42667c

Comment 13

[Jim Blandy :jimb]

Here is a clean try push for that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e827e9fa5d0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Jim Blandy :jimb]

Okay, with the patch in bug 1155438, this is green on OSX 10.6 debug JIT tests.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f65d5df4138b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f65d5df4138b