Closed Bug 1218643 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox44 --- affected

People

(Reporter: hev, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

Output "Disabled by unsupported system page size" when running on unspported platform.
Attachment #8679219 - Flags: review?(arai.unmht)
On the main OSes, can a process choose 4KiB pages on MIPS64?
(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?
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.
(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.
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 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.
Attachment #8679219 - Flags: review?(arai.unmht)
Attachment #8679219 - Attachment is obsolete: true
Summary: OdinMonkey: Update warning message for unsupported page size → OdinMonkey: Making asm.js works on non 4KiB system page size
Duplicate of this bug: 1218644
Assignee: r → nobody
Status: ASSIGNED → NEW
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.
Attachment #8681811 - Flags: review?(luke)
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.
See Also: → 903806, 1147837
Attachment #8681811 - Flags: review?(luke) → review+
Keywords: leave-open
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.
Flags: needinfo?(r)
(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!
Flags: needinfo?(r)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16727896&repo=mozilla-inbound
Flags: needinfo?(lhansen)
Noted.  :hev, note this blocks your patch too, in principle.
Flags: needinfo?(lhansen) → needinfo?(r)
(In reply to Lars T Hansen [:lth] from comment #16)
> Noted.  :hev, note this blocks your patch too, in principle.

got it, thanks.
Flags: needinfo?(r)
Adjustment to a DOM test case, now on try.
Oops sorry, I should've found this back when we added the deprecation warning.
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.
Attachment #8683036 - Flags: review?(bugs)
Attachment #8683036 - Flags: review?(bugs) → review+
Per spec, wasm uses 64 KiB pages now, and I don't think this is likely to change. Should we close as WONTFIX?
Flags: needinfo?(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'.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(luke)
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.