Some data types in Scheduling.h are excessively large

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: pbone, Assigned: pbone)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

8 months ago
No description provided.
Assignee

Comment 1

8 months ago
The tuning parameters (usually percentages) are approximate anyway, doubles
are overkill.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Attachment #9014729 - Flags: review?(sphink)
Assignee

Comment 2

8 months ago
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+
Assignee

Comment 5

8 months ago
(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.
Assignee

Comment 6

8 months ago
Attachment #9015820 - Flags: review?(sphink)
Assignee

Comment 7

8 months ago
(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+
Assignee

Updated

8 months ago
Priority: -- → P3

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5aee88f016b0
https://hg.mozilla.org/mozilla-central/rev/2398cc611aba
https://hg.mozilla.org/mozilla-central/rev/6dd61044e50a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1498177
You need to log in before you can comment on or make changes to this bug.