Try to simplify AutoEnterOOMUnsafeRegion

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
AutoEnterOOMUnsafeRegion ctor/dtor modifying the simulation's max-checks field is a bit hard to reason about and I'm not sure it's always doing the right thing. It might be simpler to add a bool flag and use that instead.

Comment 2

6 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1d50d21da50d
Simplify AutoEnterOOMUnsafeRegion by adding an explicit inUnsafeRegion_ flag to the OOM simulator. r=jonco
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&searchStr=spidermonkey&revision=1d50d21da50d91de52719decd7bf3e942a9ac89f&selectedJob=212257022

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&searchStr=spidermonkey&revision=1d50d21da50d91de52719decd7bf3e942a9ac89f&selectedJob=212257022

Backout link: https://hg.mozilla.org/integration/autoland/rev/dad98317fee0ff692f30d6f0c845a9241f0d5de4

TEST-PASS | js\src\jit-test\tests\modules\dynamic-import-module.js | Success (code 0, args "--baseline-eager") [0.3 s]
{"action": "test_start", "jitflags": "--baseline-eager", "pid": 3724, "source": "jittests", "test": "modules\\dynamic-import-module.js", "thread": "main", "time": 1542397512.124}
{"action": "test_end", "extra": {"jitflags": "--baseline-eager"}, "jitflags": "--baseline-eager", "message": "Success", "pid": 3724, "source": "jittests", "status": "PASS", "test": "modules\\dynamic-import-module.js", "thread": "main", "time": 1542397512.387}
Exit code: 1
TIMEOUT - modules\dynamic-import-oom.js
TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\modules\dynamic-import-oom.js | Timeout (code 1, args "") [150.0 s]
{"action": "test_start", "jitflags": "", "pid": 3724, "source": "jittests", "test": "modules\\dynamic-import-oom.js", "thread": "main", "time": 1542397512.373}
{"action": "test_end", "extra": {"jitflags": ""}, "jitflags": "", "message": "Timeout", "pid": 3724, "source": "jittests", "status": "FAIL", "test": "modules\\dynamic-import-oom.js", "thread": "main", "time": 1542397662.386}
INFO exit-status     : 1
INFO timed-out       : True
TEST-PASS | js\src\jit-test\tests\modules\dynamic-import-oom.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off") [1.0 s]
{"action": "test_start", "jitflags": "--ion-eager --ion-offthread-compile=off", "pid": 3724, "source": "jittests", "test": "modules\\dynamic-import-oom.js", "thread": "main", "time": 1542397662.407}
{"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-offthread-compile=off"}, "jitflags": "--ion-eager --ion-offthread-compile=off", "message": "Success", "pid": 3724, "source": "jittests", "status": "PASS", "test": "modules\\dynamic-import-oom.js", "thread": "main", "time": 1542397663.453}
Exit code: 1
TIMEOUT - modules\dynamic-import-oom.js
Flags: needinfo?(jdemooij)
Assignee

Comment 4

6 months ago
dynamic-import-oom.js becomes slower now when oomTest is testing the THREAD_TYPE_ION thread type. We do a lot more iterations, I think this is because the old code bumped maxChecks too much in the AutoEnterOOMUnsafeRegion destructor.

I'll look into it more but I may add `|jit-test| --ion-offthread-compile=off` to dynamic-import-oom.js just to unblock this - it does fix correctness issues in our OOM testing infrastructure I think.
Assignee

Comment 5

6 months ago
I added some logging and yes, before this patch we would for instance set maxChecks to 9512167 in FailureSimulator::simulateFailureAfter but then at the end of the iteration it would be 9512548 so we incorrectly added 381 to it and this would then confuse stopSimulating => HadSimulatedOOM into thinking we're done. With the patch this no longer happens so the test runs slower, hence the timeout.

Comment 6

6 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7987651658f0
Simplify AutoEnterOOMUnsafeRegion by adding an explicit inUnsafeRegion_ flag to the OOM simulator. r=jonco
Assignee

Updated

6 months ago
Flags: needinfo?(jdemooij)

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7987651658f0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.