1218643 - OdinMonkey: Making asm.js works on non 4KiB system page size

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Output "Disabled by unsupported system page size" when running on unspported platform.

Comment 1

[hev [:hev]]

Attachment: 0001-OdinMonkey-Update-warning-message-for-unsupported-pa.patch

Comment 2

[Luke Wagner [:luke]]

On the main OSes, can a process choose 4KiB pages on MIPS64?

Comment 3

[hev [:hev]]

(In reply to Luke Wagner [:luke] from comment #2)
> On the main OSes, can a process choose 4KiB pages on MIPS64?

I very much hope that 4Kib pages can works on Loongson CPU-s, but in fact is not, that will cause cache alias issue. :(

If possible, Should we evaluate the 16KiB pages feasibility for odinmonkey?

Comment 4

[Luke Wagner [:luke]]

Yes, the page size was only fixed to 4KiB because that was initially the lowest allowable heap size.  Now the minimum heap size is 64KiB (though we still allow <64KiB heaps and emit a deprecation warning).  Since we've had the deprecation warning for a year, we could now enforce the 64KiB heap minimum.  As long as cx->gcSystemPageSize() is <= 64KiB (checked in EstablishPreconditions), I think we could even remove the AsmJSPageSize constant and just use the dynamic cx->gcSystemPageSize() value.

Comment 5

[hev [:hev]]

(In reply to Luke Wagner [:luke] from comment #4)
> Yes, the page size was only fixed to 4KiB because that was initially the
> lowest allowable heap size.  Now the minimum heap size is 64KiB (though we
> still allow <64KiB heaps and emit a deprecation warning).  Since we've had
> the deprecation warning for a year, we could now enforce the 64KiB heap
> minimum.  As long as cx->gcSystemPageSize() is <= 64KiB (checked in
> EstablishPreconditions), I think we could even remove the AsmJSPageSize
> constant and just use the dynamic cx->gcSystemPageSize() value.

Good! Sounds this bug and bug 1218644 are unnecessary.

Comment 6

[Lars T Hansen [:lth]]

For another nail in the coffin, the page size is 16KB on 64-bit iOS.  Not a very important platform for us, I know, but one can hope.

Here's a report from an ARM64 user with a 64KB page size:  https://github.com/golang/go/issues/11933.  The comments in that bug suggest to me that this is not a completely unusual configuration.  Indeed this page: http://www.realworldtech.com/arm64/4/ suggests that "standard" ARM64 page sizes are 4KB and 64KB (and that suggests to me that the 16KB page on iOS is an Apple-specific size).

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8679219 [details] [diff] [review]
0001-OdinMonkey-Update-warning-message-for-unsupported-pa.patch

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

based on conversation, clearing r? for now.
and I think I'm not capable for reviewing this bug.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1218643-no-deprecation.patch

Remove support for deprecated asm.js heap size (there didn't seem to be any point to keeping the code after this); fix one test case.

Comment 10

[Lars T Hansen [:lth]]

More nails:

Bug 903806 notes that PPC uses 4KB and 64KB pages (and also notes the 64KB pages on ARM64).

Bug 1147837 notes that Sparc64 can use 8KB pages; the Wikipedia (https://en.wikipedia.org/wiki/Page_%28computer_memory%29) claims SPARC uses 8KB generally.

Comment 11

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8040c4a25b74

Comment 12

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4ea28c022f28c4f1afc1a79fb1de1452db7812
Bug 1218643 - remove support for deprecated asm.js heap length.  r=luke

Comment 13

[Lars T Hansen [:lth]]

The patch that has landed here unblocks all work that requires a static page size different from 4KB.  I'm going to leave the bug open so that it can be used as the basis for Luke's suggestion in comment 4, but I've no plans to do that work now.

:hev, you may want to reopen bug 1218644 and land the patch you had there (or something like it) to make things work on MIPS with a static page size.

Comment 14

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #13)
> The patch that has landed here unblocks all work that requires a static page
> size different from 4KB.  I'm going to leave the bug open so that it can be
> used as the basis for Luke's suggestion in comment 4, but I've no plans to
> do that work now.
> 
> :hev, you may want to reopen bug 1218644 and land the patch you had there
> (or something like it) to make things work on MIPS with a static page size.

OK, thank you!

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16727896&repo=mozilla-inbound

Comment 16

[Lars T Hansen [:lth]]

Noted.  :hev, note this blocks your patch too, in principle.

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/959ea5ec147a

Comment 18

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #16)
> Noted.  :hev, note this blocks your patch too, in principle.

got it, thanks.

Comment 19

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fa60b2c245

Comment 20

[Lars T Hansen [:lth]]

Attachment: bug1218634-domtest.patch

Adjustment to a DOM test case, now on try.

Comment 21

[Luke Wagner [:luke]]

Oops sorry, I should've found this back when we added the deprecation warning.

Comment 22

[Lars T Hansen [:lth]]

Comment on attachment 8683036 [details] [diff] [review]
bug1218634-domtest.patch

This small change is needed because the other patch on this bug changes the minimum buffer size of asm.js from 4K to 64K.

Comment 23

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a196bd265eabe5ac30b12967ca9aa5dc31bbe916
Bug 1218643 - remove support for deprecated asm.js heap length.  r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6e61d6097c166b541f513fee856d02f246742d
Bug 1218643 - correct a DOM test.  r=smaug

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a196bd265eab
https://hg.mozilla.org/mozilla-central/rev/7f6e61d6097c

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Per spec, wasm uses 64 KiB pages now, and I don't think this is likely to change. Should we close as WONTFIX?

Comment 26

[Luke Wagner [:luke]]

Actually, we now support a dynamic gc::SystemPageSize() so long as it is less than the static wasm::PageSize so I'd call this 'fixed'.

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.