Status

()

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla65
x86_64
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Assignee

Description

7 months ago
Posted patch update-cranelift (obsolete) — Splinter Review
This updates Cranemonkey to Cranelift 0.23. Major changes:
 - The new fallthrough_return patch eliminates the need for manually removing return instructions.
 - The "deref" global value type changes to "load" and the offset is on the pointer rather than on the loaded value.
 - There's a new "iadd_imm" global value.
 - The heap base is now considered readonly, allowing redundant loads of the base pointer from TLS to be eliminated.
 - This version has the jump table feature, but it's disabled in this patch as it needs additional changes.
Attachment #9023623 - Flags: review?(bbouvier)
Assignee

Comment 1

7 months ago
Posted patch call-indirect-globals (obsolete) — Splinter Review
This removes the workaround for Cranelift #491.
Assignee: nobody → sunfish
Attachment #9023624 - Flags: review?(bbouvier)
Assignee

Comment 2

7 months ago
Posted patch mach-vendor-rust (obsolete) — Splinter Review
This runs "mach vendor rust" and contains the actual Cranelift 0.23 update. I split it into a separate patch to make reviewing easier, but I can merge the patches to keep the patches self-contained.
Attachment #9023626 - Flags: review?(bbouvier)
Assignee

Updated

7 months ago
Attachment #9023624 - Attachment is patch: true
Attachment #9023624 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 9023623 [details] [diff] [review]
update-cranelift

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

Many things look so much more natural now (loading from TlsBase + offset, in particular). LGTM, thanks.

::: js/src/wasm/cranelift/src/compile.rs
@@ -122,5 @@
> -    /// Remove the trailing return instruction from the current function to make room for a custom
> -    /// epilogue.
> -    ///
> -    /// Return the new function size in bytes, adjusted from size.
> -    fn remove_return_inst(&mut self, size: CodeOffset) -> CodeOffset {

yay

::: js/src/wasm/cranelift/src/cpu.rs
@@ +38,5 @@
>  }
>  
>  impl From<settings::SetError> for BasicError {
>      fn from(err: settings::SetError) -> BasicError {
> +        BasicError::new(err.to_string())

I think we can do even better by just reusing the error trait that's used in Cranelift, so we don't even have the BasicError structure at all? (maybe as a follow up)

::: js/src/wasm/cranelift/src/wasm2clif.rs
@@ +187,5 @@
>      fn get_table(&mut self, func: &mut ir::Function, table: TableIndex) -> TableInfo {
>          // Allocate all tables up to the requested index.
> +        while self.tables.len() <= table.index() {
> +            let wtab = self.env.table(TableIndex::new(self.tables.len()));
> +            let vmctx = self.get_vmctx_gv(func);

This can be hoisted above the loop, for clarity.

@@ +428,5 @@
>  
>      fn make_heap(&mut self, func: &mut ir::Function, index: MemoryIndex) -> ir::Heap {
> +        assert_eq!(
> +            index,
> +            MemoryIndex::new(0),

This can be simplified as assert_eq!(index.index(), 0, ...); I think?

@@ +540,5 @@
>          let wsig = self.env.signature(sig_index);
>  
>          // TODO: When compiling asm.js, the table index in inferred from the signature index.
>          // Currently, WebAssembly doesn't support multiple tables. That may change.
> +        assert_eq!(table_index, TableIndex::new(0));

ditto
Attachment #9023623 - Flags: review?(bbouvier) → review+
Attachment #9023624 - Flags: review?(bbouvier) → review+
Comment on attachment 9023626 [details] [diff] [review]
mach-vendor-rust

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

From a look at the Cargo.lock file, this is accurate. rs=me
Attachment #9023626 - Flags: review?(bbouvier) → review+
Assignee

Comment 5

7 months ago
Posted patch update-cranelift (obsolete) — Splinter Review
Review comments addressed.
Attachment #9023623 - Attachment is obsolete: true
Assignee

Comment 6

7 months ago
Attachment #9023624 - Attachment is obsolete: true
Assignee

Comment 7

7 months ago
Attachment #9023626 - Attachment is obsolete: true
Assignee

Comment 8

7 months ago
Comment on attachment 9023684 [details] [diff] [review]
update-cranelift

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

Carrying forward r+.
Attachment #9023684 - Flags: review+
Assignee

Comment 9

7 months ago
Comment on attachment 9023685 [details] [diff] [review]
call-indirect-globals

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

Carrying forward r+.
Attachment #9023685 - Flags: review+
Assignee

Comment 10

7 months ago
Comment on attachment 9023687 [details] [diff] [review]
mach-vendor-rust

Carrying forward r+.
Attachment #9023687 - Attachment is patch: true
Attachment #9023687 - Attachment mime type: application/octet-stream → text/plain
Attachment #9023687 - Flags: review+
Assignee

Updated

7 months ago
Keywords: checkin-needed

Comment 11

7 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe8e6a201ed
[Cranelift] Update to Cranelift 0.23. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/29649174d3e7
[Cranelift] Use the Cranelift global values for tables. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/027bf8dee1e5
Run mach-vendor-rust to update Cranelift. rs=bbouvier
Keywords: checkin-needed
Backed out 3 changesets (bug 1505777) for linux leakcheck failures on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8811545863bb0b3faf036d57a3b5480e7694e64

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=210673669&revision=f1e6ccbef48d843bff32d5b1c6d222041de98436

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=210673715&repo=mozilla-inbound&lineNumber=1832

Log snippet: 

[task 2018-11-08T22:50:06.455Z] 22:50:06     INFO - TEST-INFO | Main app process: exit 0
[task 2018-11-08T22:50:06.456Z] 22:50:06     INFO - runtests.py | Application ran for: 0:00:17.673165
[task 2018-11-08T22:50:06.457Z] 22:50:06     INFO - zombiecheck | Reading PID log: /tmp/tmpGADrV9pidlog
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - ==> process 1057 launched child process 1078
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - zombiecheck | Checking for orphan process with PID: 1078
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - Stopping web server
[task 2018-11-08T22:50:06.479Z] 22:50:06     INFO - Stopping web socket server
[task 2018-11-08T22:50:06.500Z] 22:50:06     INFO - Stopping ssltunnel
[task 2018-11-08T22:50:06.522Z] 22:50:06     INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.522Z] 22:50:06     INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | gpu process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.526Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.527Z] 22:50:06     INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1057
[task 2018-11-08T22:50:06.528Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.529Z] 22:50:06     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2018-11-08T22:50:06.531Z] 22:50:06     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2018-11-08T22:50:06.532Z] 22:50:06     INFO -    0 |TOTAL                                 |       27        4| 3977199        1|
[task 2018-11-08T22:50:06.540Z] 22:50:06     INFO - 1595 |nsTArray_base                         |        4        4| 1797722        1|
[task 2018-11-08T22:50:06.542Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.542Z] 22:50:06     INFO - nsTraceRefcnt::DumpStatistics: 1733 entries
[task 2018-11-08T22:50:06.543Z] 22:50:06     INFO - TEST-INFO | leakcheck | default process: leaked 1 nsTArray_base
[task 2018-11-08T22:50:06.544Z] 22:50:06    ERROR - TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray_base)
[task 2018-11-08T22:50:06.544Z] 22:50:06     INFO - runtests.py | Running tests: end.
Flags: needinfo?(sunfish)

Comment 13

7 months ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbdabbc30f33
[Cranelift] Update to Cranelift 0.23. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/400c83b3756b
[Cranelift] Use the Cranelift global values for tables. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3aff6770a5
Run mach-vendor-rust to update Cranelift. rs=bbouvier
Seems that Bug 1501102 was the culprit for the leakchecks. This bug was relanded.
Flags: needinfo?(sunfish)

Comment 15

7 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1450f97acc2
Backed out 3 changesets for linux build failures on a CLOSED TREE
Assignee

Comment 16

7 months ago
This updates the patch with a fix for the tup build: new_types.rs is no longer being generated, so remove it from python/mozbuild/mozbuild/backend/cargo_build_defs.py.

Carrying forward r+.
Attachment #9023684 - Attachment is obsolete: true
Attachment #9023975 - Flags: review+
Assignee

Updated

7 months ago
Keywords: checkin-needed

Comment 18

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3fb1fce6a9
[Cranelift] Update to Cranelift 0.23. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb48d47b208
[Cranelift] Use the Cranelift global values for tables. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2013de59e87
Run mach-vendor-rust to update Cranelift. rs=bbouvier
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.