804857 - be optimistic about source compression ratio for memory purposes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

This is a bug to implement the suggestion in bug 776741 comment 16. We will allocate less memory that we may need at the beginning of source compression and increase it if needed.

Comment 1

[:Benjamin Peterson]

Attachment: allocate memory in compression thread and make clients check for error

This patch refactors source compression, so that the compression buffer is allocated in the compression thread. It also creates a flag for the compression thread to indicate OOM.

Comment 2

[Andrew McCreight [:mccr8]]

Now the question is how you can write some very uncompressable JS to check the case when the optimism fails. ;)  I guess something that is mostly a giant randomly generated string.

Comment 3

[:Benjamin Peterson]

Attachment: allocate memory in compression thread and make clients check for error

Small tweaks on this.

Comment 4

[:Benjamin Peterson]

Attachment: use smaller initial buffer

Comment 5

[:Benjamin Peterson]

It doesn't seem we can actually see what the effect of these patches on MaxHeap is without landing them. :(

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Comment on attachment 674558 [details] [diff] [review]
allocate memory in compression thread and make clients check for error

Review of attachment 674558 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsscript.h
@@ +1061,5 @@
>  
>    private:
>      void destroy(JSRuntime *rt);
> +    bool compressed() const { return compressedLength_ != 0; }
> +    size_t sizeOfData() const {

Can you call this |computedSizeOfData|?  We have a convention that |sizeOfFoo| functions measure the size of something with a JSMallocSizeOfFun, and |ComputedSizeOfFoo| functions compute a size analytically.

@@ +1093,5 @@
>   * To use it, you have to have a SourceCompressionToken, tok, with tok.ss and
>   * tok.chars set to the proper values. When the SourceCompressionToken is
> + * destroyed, it makes sure the compression is complete. If you are about to
> + * successfully exit the scope of tok, you should call and check the return
> + * value of SourceCompressionToken::complete(). It returns false if allocation

Can we make it impossible to forget this?  E.g. set a flag in complete(), and then assert that it is set in the destructor?

Comment 7

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> @@ +1093,5 @@
> >   * To use it, you have to have a SourceCompressionToken, tok, with tok.ss and
> >   * tok.chars set to the proper values. When the SourceCompressionToken is
> > + * destroyed, it makes sure the compression is complete. If you are about to
> > + * successfully exit the scope of tok, you should call and check the return
> > + * value of SourceCompressionToken::complete(). It returns false if allocation
> 
> Can we make it impossible to forget this?  E.g. set a flag in complete(),
> and then assert that it is set in the destructor?

I wish. We want to allow people not to check it if they reach an error return in the compiler, though, and the destructor can't distinguish those cases.

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Comment on attachment 674559 [details] [diff] [review]
use smaller initial buffer

Review of attachment 674559 [details] [diff] [review]:
-----------------------------------------------------------------

This patch would be easier to understand if you had split the refactoring parts out first, and then resized the buffer in a second patch.

::: js/src/jsscript.cpp
@@ +960,5 @@
> +                if (comp.outWritten() == nbytes) {
> +                    cont = false;
> +                    break;
> +                }
> +                void *newmem = js_realloc(ss->data.compressed, nbytes);

A comment here -- e.g. "a half-sized buffer wasn't enough, try a full-sized buffer" -- would be nice.  (This would be a logical follow-up to the "Try to keep..., first." comment above.)

@@ +981,5 @@
> +            compressedLength = 0;
> +            if (stop) {
> +                void *newmem = js_realloc(ss->data.compressed, nbytes);
> +                if (!newmem)
> +                    return false;

A comment saying "ss->data.compressed is freed by the caller" would be nice here.

@@ +998,5 @@
>          PodCopy(ss->data.source, tok->chars, ss->length());
>      } else {
>          // Shrink the buffer to the size of the compressed data.
>          void *newmem = js_realloc(ss->data.compressed, compressedLength);
> +        if (!newmem)

Ditto.

(Alternatively, you could just free in the exit paths in this function.  I think the ugliness of the duplication is outweighed by the clarity of freeing the memory right next to where the allocation failed.)

Comment 9

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> Now the question is how you can write some very uncompressable JS to check
> the case when the optimism fails. ;)  I guess something that is mostly a
> giant randomly generated string.

I forgot to say:  I'd like a test that does this!  r=me with that.  Thanks.

Comment 10

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > Now the question is how you can write some very uncompressable JS to check
> > the case when the optimism fails. ;)  I guess something that is mostly a
> > giant randomly generated string.
> 
> I forgot to say:  I'd like a test that does this!  r=me with that.  Thanks.

We already have such tests. jit-test/tests/basic/function-tosource-bug779694.js for example.

Comment 11

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> We already have such tests.

Great!  Can you double-check with the debugger that you're hitting the new path with this test, and also add a comment to that test explaining that it hits this path?

Comment 12

[:Benjamin Peterson]

Attachment: use smaller initial buffer

Here is the second patch with review comments addressed and slightly refactored.

Comment 13

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb516a4afc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c27fd77dbaa

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfb516a4afc2
https://hg.mozilla.org/mozilla-central/rev/1c27fd77dbaa

Comment 16

[Nicholas Nethercote [:njn] (On Long Service Leave)]

http://graphs-new.mozilla.org/graph.html#tests=[[29,1,17],[29,1,7],[29,1,22],[29,1,6],[29,1,18],[29,1,8]]&sel=none&displayrange=7&datatype=running  seems to indicate an improvement on most platforms, assuming I'm reading it correctly:

- CentOS:     -2.1%
- CentOS x64: -1.5%
- MacOS 10.7: -1.0%
- WinNT:    : -0.0%

The MacOS 10.7 one was bimodal, and the -1.0% reduction is from the lower of the two values.