Closed Bug 863526 Opened 7 years ago Closed 7 years ago

Attempt to GC before the StoreBuffer overflows

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
Brian's research in Bug 851057 found that watching for when the store buffer is about to overflow and doing a minor GC as soon as possible after that point is faster than taking occasional overflows. This patch takes the 7/8 fullness ratio from that patch, but implements the logic on the common base class so we get the benefit for all store buffers.
Attachment #739373 - Flags: review?(wmccloskey)
Attachment #739373 - Flags: feedback?(bhackett1024)
Comment on attachment 739373 [details] [diff] [review]
v0

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

This doesn't actually trigger the operation callback. It seems like you would want to do that.
(In reply to Bill McCloskey (:billm) from comment #1)
> This doesn't actually trigger the operation callback. It seems like you
> would want to do that.

D'oh! I decided to rewrite the implementation rather than copying because I changed the name. Thanks for catching that!
Attachment #739373 - Attachment is obsolete: true
Attachment #739373 - Flags: review?(wmccloskey)
Attachment #739373 - Flags: feedback?(bhackett1024)
Attachment #739687 - Flags: review?(wmccloskey)
Attachment #739687 - Flags: feedback?(bhackett1024)
Comment on attachment 739687 [details] [diff] [review]
v1: actually trigger the operation callback.

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

I just noticed that we're doing a function call every time we add something to the store buffer. I wonder if that affects performance.

::: js/src/gc/StoreBuffer.h
@@ +117,5 @@
> + * How full we allow a store buffer to become before we request a MinorGC.
> + *
> + * Note: clang-3.2 segfaults if this is defined as a class variable.
> + */
> +const static double HighwaterRatio = 7.0 / 8.0;

Could this go in the .cpp file?

@@ +358,5 @@
>      JSRuntime *runtime;
>  
>      void *buffer;
>  
> +    bool aboutToOverflow;

I think you need to initialize this in the constructor. Also, you probably want to set it to false in disable().
Attachment #739687 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #3)
> I just noticed that we're doing a function call every time we add something
> to the store buffer. I wonder if that affects performance.

I expect so. I created them out-of-line so that wouldn't have to do a full recompile every test cycle while creating and debugging them. Now would be a great time to inline them.

Brian is also doing a full unaligned ABI call from ion for each of these(!) which I expect we should also inline.
https://hg.mozilla.org/mozilla-central/rev/8696520f1e90
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Attachment #739687 - Flags: feedback?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.