Closed Bug 1496699 Opened 2 years ago Closed 2 years ago

Some data types in Scheduling.h are excessively large

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(3 files)

No description provided.
The tuning parameters (usually percentages) are approximate anyway, doubles
are overkill.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Attachment #9014729 - Flags: review?(sphink)
Some of these values were uint64_t which would not have made sense on 32bit
systems.
Attachment #9014730 - Flags: review?(sphink)
Comment on attachment 9014730 [details] [diff] [review]
Bug 1496699 - Use size_t for memory sizes

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

::: js/src/jit-test/tests/gc/gcparam.js
@@ +18,2 @@
>      gcparam(key, value);
>      assertEq(gcparam(key), value);

Huh. Weird test. Should it be

  const value = 1000000;
  try {
    gcparam(key, value);
    assertEq(gcparam(key), value);
  } except {
    assertEq(gcparam(key), prev);
  }
  gcparam(key, prev);

? If you just change it to 1024, it seems like it isn't testing large values anymore.
Attachment #9014730 - Flags: review?(sphink) → review+
Comment on attachment 9014729 [details] [diff] [review]
Bug 1496699 - Convert some doubles to floats

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

I suppose this is ok. I doubt the size difference matters much, and I'm never clear on the performance tradeoffs between float and double, but it seems unlikely to hurt.
Attachment #9014729 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Comment on attachment 9014730 [details] [diff] [review]
> Bug 1496699 - Use size_t for memory sizes
> 
> Review of attachment 9014730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/gc/gcparam.js
> @@ +18,2 @@
> >      gcparam(key, value);
> >      assertEq(gcparam(key), value);
> 
> Huh. Weird test. Should it be
> 
>   const value = 1000000;
>   try {
>     gcparam(key, value);
>     assertEq(gcparam(key), value);
>   } except {
>     assertEq(gcparam(key), prev);
>   }
>   gcparam(key, prev);

That's probably a good idea.

> ? If you just change it to 1024, it seems like it isn't testing large values
> anymore.

testLargeParamValue is only used in two places, for the highFrequencyLowLimit and highFrequencyHighLimit, these are in megabytes, so 1024 is a big number.

There's more I've changed here so I'll make it a seperate patch.
Attachment #9015820 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> Comment on attachment 9014729 [details] [diff] [review]
> Bug 1496699 - Convert some doubles to floats
> 
> Review of attachment 9014729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suppose this is ok. I doubt the size difference matters much, and I'm
> never clear on the performance tradeoffs between float and double, but it
> seems unlikely to hurt.

Me neither, however smaller things will take up less memory while at rest.  However what did bother me was that on 32bit systems we'd be using two registers and extra work to manage the 64bit integer values.

Not that these calculations are all that frequent, I mostly fixed it because I wanted to add new parameters and use the _right_ types for them. but also wanted them to fit in with whats already there.
Comment on attachment 9015820 [details] [diff] [review]
Bug 1496699 - Refactor gcparam.js test

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

::: js/src/jit-test/tests/gc/gcparam.js
@@ +6,4 @@
>  
>  function testChangeParam(key) {
>      let prev = gcparam(key);
> +    let vaue = prev - 1;

*value
Attachment #9015820 - Flags: review?(sphink) → review+
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.