Last Comment Bug 804857 - be optimistic about source compression ratio for memory purposes
: be optimistic about source compression ratio for memory purposes
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: general
:
Mentors:
: 804897 (view as bug list)
Depends on:
Blocks: 776741
  Show dependency treegraph
 
Reported: 2012-10-23 17:45 PDT by :Benjamin Peterson
Modified: 2012-10-25 09:15 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
allocate memory in compression thread and make clients check for error (11.19 KB, patch)
2012-10-23 17:49 PDT, :Benjamin Peterson
no flags Details | Diff | Review
allocate memory in compression thread and make clients check for error (11.14 KB, patch)
2012-10-23 23:12 PDT, :Benjamin Peterson
n.nethercote: review+
Details | Diff | Review
use smaller initial buffer (8.60 KB, patch)
2012-10-23 23:12 PDT, :Benjamin Peterson
n.nethercote: review+
Details | Diff | Review
use smaller initial buffer (9.39 KB, patch)
2012-10-24 15:09 PDT, :Benjamin Peterson
benjamin: review+
Details | Diff | Review

Description :Benjamin Peterson 2012-10-23 17:45:54 PDT
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 2012-10-23 17:49:05 PDT
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.
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-10-23 19:01:56 PDT
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 2012-10-23 23:12:02 PDT
Created attachment 674558 [details] [diff] [review]
allocate memory in compression thread and make clients check for error

Small tweaks on this.
Comment 4 :Benjamin Peterson 2012-10-23 23:12:27 PDT
Created attachment 674559 [details] [diff] [review]
use smaller initial buffer
Comment 5 :Benjamin Peterson 2012-10-23 23:13:51 PDT
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 vacation until July 11) 2012-10-24 09:55:11 PDT
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 2012-10-24 10:17:19 PDT
(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 vacation until July 11) 2012-10-24 10:25:42 PDT
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 vacation until July 11) 2012-10-24 13:09:35 PDT
> 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 2012-10-24 14:30:13 PDT
(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 vacation until July 11) 2012-10-24 14:50:22 PDT
> 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 2012-10-24 15:09:32 PDT
Created attachment 674854 [details] [diff] [review]
use smaller initial buffer

Here is the second patch with review comments addressed and slightly refactored.
Comment 14 Ginn Chen 2012-10-24 22:26:35 PDT
*** Bug 804897 has been marked as a duplicate of this bug. ***
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-10-25 09:15:54 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.