Closed Bug 804857 Opened 8 years ago Closed 8 years ago

be optimistic about source compression ratio for memory purposes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Benjamin, Unassigned)

References

Details

(Whiteboard: [MemShrink])

Attachments

(2 files, 2 obsolete files)

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.
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.
Whiteboard: [MemShrink]
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.
Small tweaks on this.
Attachment #674466 - Attachment is obsolete: true
Attached patch use smaller initial buffer (obsolete) — Splinter Review
It doesn't seem we can actually see what the effect of these patches on MaxHeap is without landing them. :(
Attachment #674558 - Flags: review?(n.nethercote)
Attachment #674559 - Flags: review?(n.nethercote)
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?
Attachment #674558 - Flags: review?(n.nethercote) → review+
(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 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.)
Attachment #674559 - Flags: review?(n.nethercote) → review+
> 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.
(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.
> 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?
Here is the second patch with review comments addressed and slightly refactored.
Attachment #674559 - Attachment is obsolete: true
Attachment #674854 - Flags: review+
Duplicate of this bug: 804897
https://hg.mozilla.org/mozilla-central/rev/dfb516a4afc2
https://hg.mozilla.org/mozilla-central/rev/1c27fd77dbaa
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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.
You need to log in before you can comment on or make changes to this bug.