Skip to content

core: memory limits & callbacks#6914

Merged
piscisaureus merged 18 commits into
denoland:masterfrom
mraerino:feat/memory-limits
Aug 12, 2020
Merged

core: memory limits & callbacks#6914
piscisaureus merged 18 commits into
denoland:masterfrom
mraerino:feat/memory-limits

Conversation

@mraerino
Copy link
Copy Markdown
Contributor

@mraerino mraerino commented Jul 29, 2020

Fixes #6916

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jul 29, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you forgot to set V8_FROM_SOURCE?

@mraerino mraerino changed the title WIP: Feat/memory limits WIP: core: memory limits & callbacks Jul 30, 2020
@mraerino
Copy link
Copy Markdown
Contributor Author

mraerino commented Aug 4, 2020

@piscisaureus this needs a release in rusty_v8. were you going to make one soon?

@piscisaureus
Copy link
Copy Markdown
Member

Yes this week.

@mraerino mraerino changed the title WIP: core: memory limits & callbacks core: memory limits & callbacks Aug 10, 2020
@mraerino mraerino marked this pull request as ready for review August 10, 2020 15:21
@mraerino
Copy link
Copy Markdown
Contributor Author

did this fail because of a flaky test?

@ry
Copy link
Copy Markdown
Member

ry commented Aug 10, 2020

@mraerino Currently I don't think we have any flaky tests - but who knows it's always possible. Try pushing an empty commit to re-trigger CI.

My guess would be that it's related to your patch. I've pulled out the relevant failure logs from Github Actions:

2020-08-10T16:51:55.4750378Z test timeoutCancelSuccess ... test lock_check_ok2 ... ok
2020-08-10T16:51:55.7768303Z error: Uncaught AssertionError: Assertion failed.
2020-08-10T16:51:55.7769007Z ok (602ms)
2020-08-10T16:51:55.7779412Z     at assert (rt/06_util.js:33:13)
2020-08-10T16:51:55.7784881Z     at setGlobalTimeout (rt/11_timers.js:305:5)
2020-08-10T16:51:55.7820619Z     at setOrClearGlobalTimeout (rt/11_timers.js:349:7)
2020-08-10T16:51:55.8065898Z     at unschedule (rt/11_timers.js:398:9)
2020-08-10T16:51:55.8066362Z     at clearTimer (rt/11_timers.js:514:5)
2020-08-10T16:51:55.8067075Z     at clearTimeout (rt/11_timers.js:523:5)
2020-08-10T16:51:55.8067667Z     at timeoutCancelMultiple (file:///D:/a/deno/deno/cli/tests/unit/timers_test.ts:84:3)
2020-08-10T16:51:55.8068022Z     at asyncOpSanitizer (rt/40_testing.js:34:13)
2020-08-10T16:51:55.8068751Z     at Object.resourceSanitizer [as fn] (rt/40_testing.js:68:13)
2020-08-10T16:51:55.8070940Z     at TestRunner.[Symbol.asyncIterator] (rt/40_testing.js:240:24)
2020-08-10T16:51:55.8071934Z error: Uncaught ConnectionReset: An existing connection was forcibly closed by the remote host. (os error 10054)  
2020-08-10T16:51:55.8072673Z     at unwrapResponse (rt/10_dispatch_minimal.js:53:13)
2020-08-10T16:51:55.8090882Z     at sendAsync (rt/10_dispatch_minimal.js:96:12)
2020-08-10T16:51:55.8091311Z     at async read (rt/12_io.js:99:19)
2020-08-10T16:51:55.8091647Z     at async readDelim (file:///D:/a/deno/deno/std/io/bufio.ts:649:20)
2020-08-10T16:51:55.8091997Z     at async readStringDelim (file:///D:/a/deno/deno/std/io/bufio.ts:699:20)
2020-08-10T16:51:55.8092339Z     at async readLines (file:///D:/a/deno/deno/std/io/bufio.ts:709:3)
2020-08-10T16:51:55.8092694Z     at async runTestsForPermissionSet (file:///D:/a/deno/deno/cli/tests/unit/unit_test_runner.ts:142:22)
2020-08-10T16:51:55.8093047Z     at async masterRunnerMain (file:///D:/a/deno/deno/cli/tests/unit/unit_test_runner.ts:203:20)
2020-08-10T16:51:55.8285318Z test timeoutCancelMultiple ... test js_unit_tests ... FAILED

@mraerino
Copy link
Copy Markdown
Contributor Author

ok, i can't explain the failures. between the two runs different things failed, so they seem non-deterministic.

my changes only add new interfaces to deno_core that are not being used in cli

@ry
Copy link
Copy Markdown
Member

ry commented Aug 11, 2020

@mraerino Looks like it was indeed something flaky. Build is good. Reviewing now...

Comment thread core/core_isolate.rs
Copy link
Copy Markdown
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thank you @mraerino !

Just going to give you a chance to add AtomicUsize to the test - or not if you disagree.

@mraerino mraerino requested review from piscisaureus and ry August 11, 2020 22:13
Comment thread core/core_isolate.rs Outdated

type JSErrorCreateFn = dyn Fn(JSError) -> ErrBox;

type PinnedRefCell = Pin<Box<RefCell<dyn Any>>>;
Copy link
Copy Markdown
Member

@piscisaureus piscisaureus Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious question - what does a Pin around a RefCell (boxed or not) achieve?
I'd think that, since a RefCell allows interior mutability, this kinda negates the effect of the pin.

(In other words - why do you need to pin this?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. removed.

Comment thread core/core_isolate.rs
/// The return value of the closure is set as the new limit.
pub fn add_near_heap_limit_callback<C>(&mut self, cb: C)
where
C: FnMut(usize, usize) -> usize + 'static,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good.

Comment thread core/core_isolate.rs
Copy link
Copy Markdown
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mraerino
Copy link
Copy Markdown
Contributor Author

Now that you say this:

Ah I see you've limited it to support only one callback.. That's ok.

I realized a small thing was missing: #7025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core: Allow fine-grained heap memory control

4 participants