Note: There are a few cases of duplicates in user autocompletion which are being worked on.

be optimistic about source compression ratio for memory purposes

RESOLVED FIXED in mozilla19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Unassigned)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 674466 [details] [diff] [review]
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.
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.
(Reporter)

Comment 3

5 years ago
Created attachment 674558 [details] [diff] [review]
allocate memory in compression thread and make clients check for error

Small tweaks on this.
Attachment #674466 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
Created attachment 674559 [details] [diff] [review]
use smaller initial buffer
(Reporter)

Comment 5

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

Updated

5 years ago
Attachment #674558 - Flags: review?(n.nethercote)
(Reporter)

Updated

5 years ago
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+
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 10

5 years ago
(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?
(Reporter)

Comment 12

5 years ago
Created attachment 674854 [details] [diff] [review]
use smaller initial buffer

Here is the second patch with review comments addressed and slightly refactored.
Attachment #674559 - Attachment is obsolete: true
Attachment #674854 - Flags: review+
(Reporter)

Comment 13

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

Updated

5 years ago
Duplicate of this bug: 804897
https://hg.mozilla.org/mozilla-central/rev/dfb516a4afc2
https://hg.mozilla.org/mozilla-central/rev/1c27fd77dbaa
Status: NEW → RESOLVED
Last Resolved: 5 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.