Implement "eager evaluation" in the REPL

[benjamingr]

Hi, V8 has a basic way to detect side-effects and they use it for eager evaluation in the devtools.

It would be cool to allow for similar behaviour in our REPL. For clarity, here is the image of Chrome 68's behaviour:

_{(Image copyright (CC BY 3.0) Google Developer blog "What's New In DevTools (Chrome 68)" by Kayce Basques @kaycebasques )}

@nodejs/repl

[BridgeAR]

I like the idea.

[BridgeAR]

The implementation might not be easy to prevent side effects though. Do you know how these are prevented in Chrome?

[benjamingr]

@BridgeAR they use the function I linked to above in SharedFunctionInfo::SideEffectState BuiltinGetSideEffectState and time the evaluation out after 500ms

[devsnek]

as a sidenote we would need to start properly marking our natively defined functions on whether they have side effects or not.

[benjamingr]

@devsnek

as a sidenote we would need to start properly marking our natively defined functions on whether they have side effects or not.

IIUC only as progressive enhancement - things that weren't tagged would not eager-evaluate and the users would get the same UX they do today.

[hashseed]

The implementation might not be easy to prevent side effects though. Do you know how these are prevented in Chrome?

V8 defines whitelists for bytecodes and builtins. During side-effect free mode, if we execute bytecode or builtins that are not whitelisted, we abort. For a greylist of bytecodes and builtins, we only allow operations on objects that have been created during side-effect free mode, to ensure that these changes do not escape.

Bindings defined through the V8 API are generally not whitelisted unless explicitly marked as such.

[hashseed]

Note that side-effect free mode is only accessible through the DevTools protocol right now, through Runtime.evaluate and Debugger.evaluateOnCallFrame.

[benjamingr]

@hashseed would it be possible to expose it in a way where Node.js can use side effect free mode?

I think this could be really cool to have in our REPL

[hashseed]

You mean as option to v8::Script::Run? I'd be open to that. Then again Runtime.evaluate essentially just does that.

[benjamingr]

Seeing @jdalton recently joined @nodjes/repl - he didn't get the ping about this before (I think?).

Regarding v8::Script::Run - sure - I'm not familiar with Runtime.evaluate or if we can even use inspector functions in the REPL (even if we run an inspector in the background).

[addaleax]

@benjamingr We currently use inspector functions in the REPL when they’re available for accessing pseudo-global-scope let and const variables, so that might be the easier route to go (and doesn’t require explicit V8 support being added in the C++ API)

[hashseed]

I prefer the route through inspector because it already exists and has coverage via Chrome Devtools.

[BridgeAR]

@rubys is this something you might be interested in?

[devsnek]

fwiw https://github.com/nodejs/repl has this support already.

[jdalton]

@devsnek Would you please post links to the relevant commits or code.

[devsnek]

@jdalton https://github.com/nodejs/repl/blob/e06c75c8788c90b056c85d743cebea087d0cc98c/src/repl.js#L125-L202

[rubys]

@devsnek what's the status/plans for https://github.com/nodejs/repl ? I see that my "multiline/recoverable" changes are stalled: nodejs/repl#16 . I'm interested in helping. I'm particularly interested in #19570

[devsnek]

@rubys its moving, albeit slowly 😄. i haven't really done anything with it since node started aborting on Runtime.evaluate (#22157)

i'm currently rebasing some local code for vm which will enable #19570, then this weekend i'll try to get around to the repl prs but the aborting does make it difficult to properly test...

@rubys if you join the @nodejs/repl team you should get write access to nodejs/repl and be able to commit and do prs and whatnot. the more the merrier :)

[antsmartian]

Worth noting that when we deal with large objects, actually node-repl gets stuck. See : nodejs/repl#9

[alexkozy]

I believe that node in master branch should not abort any more 😄
b1e2612

[Trott]

@devsnek Are you still working on this? If not, should we mark it as stalled and/or help wanted?

[cactysman]

Any updates on this? 🤔

[devsnek]

usable repl is here: https://github.com/nodejs/repl

i don't really have a plan for merging it into core right now, but someone else is always welcome to take that up.

[antsmartian]

@metaa There was a attempt and a open PR here: #22875. I will try to close this when I get free time.