When implementing functions in builtins/*.js, sometimes we require lower level functionality. For example, in the current Array.from, we use `result[k] = value`. The spec requires [[DefineOwnProperty]] operation here. However, current implementation behaves as [[Put]]. So if we implement Array.prototype[k] getter/setter, the difference is observable. Ideally, reaching here, we would like to use put_by_val_direct bytecode. However, there's no syntax to generate it directly. So I propose introducing bytecode level intrinsics into JSC BytecodeCompiler. Like @call, @apply, we introduce a new node, Intrinsic. These are generated when calling appropriate private symbols in privileged code. AST parser detects them and generates Intrinsic nodes and BytecodeCompiler detects them and generate required bytecodes. For example, I'm supposing the following code in builtins/. @putByValDirect(result, k, value); And AST parser & bytecode generator detects them and generate `put_by_val_direct`.
Sounds like a good idea!
(In reply to comment #1) > Sounds like a good idea! Yay! So I'll implement it :D
Created attachment 251116 [details] Prototype Initial prototype.
Created attachment 251126 [details] Patch
Committed r182997: <http://trac.webkit.org/changeset/182997>
(In reply to comment #5) > Committed r182997: <http://trac.webkit.org/changeset/182997> It made a JSC test assert on all debug bots, see https://build.webkit.org/waterfall for details. Could you possibly check and fix this regression?
Why was this committed when both Mac EWS showed red?
Wait a second. While I think it's a good idea for builtins to use intrinsics to differentiate between [[Put]] and [[DefineOwnProperty]], I think you missed two things: (1) Currently, builtins convert *all* bracket access to put_by_val_direct: RegisterID* BytecodeGenerator::emitPutByVal(RegisterID* base, RegisterID* property, RegisterID* value) { UnlinkedArrayProfile arrayProfile = newArrayProfile(); if (m_isBuiltinFunction) emitOpcode(op_put_by_val_direct); (2) Many builtins other than Array.from require the put_by_val_direct behavior. I think you need to figure out why we have this bug despite (1), and I think you should deploy your new intrinsic everywhere for (2), and remove the hack that says that all bracket access in a builtin is a put_by_val_direct.
Re-opened since this is blocked by bug 143957
No need for a rollout, since the regression will be fixed by https://bugs.webkit.org/show_bug.cgi?id=143947, but I think it's important to address the two points above.
(In reply to comment #8) > Wait a second. While I think it's a good idea for builtins to use intrinsics > to differentiate between [[Put]] and [[DefineOwnProperty]], I think you > missed two things: > > (1) Currently, builtins convert *all* bracket access to put_by_val_direct: > > RegisterID* BytecodeGenerator::emitPutByVal(RegisterID* base, RegisterID* > property, RegisterID* value) > { > UnlinkedArrayProfile arrayProfile = newArrayProfile(); > if (m_isBuiltinFunction) > emitOpcode(op_put_by_val_direct); > > (2) Many builtins other than Array.from require the put_by_val_direct > behavior. > > I think you need to figure out why we have this bug despite (1), and I think > you should deploy your new intrinsic everywhere for (2), and remove the hack > that says that all bracket access in a builtin is a put_by_val_direct. Ah, sorry for confusing. As described in ChangeLog, after investigating the code, I found that assignments in the builtins are compiled into put_by_val. Quoted from the ChangeLog, > Currently, Array.from implementation works fine without this patch. > This is because when the target code is builtin JS, > BytecodeGenerator emits put_by_val_direct instead of put_by_val. > This solves the above issue. However, instead of solving this issue, > it raises another issue; There's no way to emit `[[Set]]` operation. > `[[Set]]` operation is actually used in the spec (Array.from's "length" is set by `[[Set]]`). > So to implement it precisely, introducing bytecode level intrinsics is necessary. > > In the subsequent fixes, we'll remove that special path emitting put_by_val_direct > for `result[k] = value` under builtin JS environment. Instead of that special handling, > use bytecode intrinsics instead. It solves problems and it is more intuitive > because written JS code in builtin works as the same to the usual JS code. So this patch itself doesn't change the current behavior. And I'm trying to fix the current Array.from behavior in the separate from this patch(implementing bytecode intrinsic system) Array.from needs [[DefineOwnProperty]] and [[Set]]. So anyway, performing both is needed. Currently, I think removing the current hack emitting put_by_val_direct is preferable because it is intuitive that witten JS code in builtins works as the same to the usual JS code. However, [[Set]] use is relatively limited compared to [[DefineOwnProperty]] in builtins. Object.assign uses [[Set]] very frequently. But Array.prototype.xxx uses [[Set]] mainly only for "length" setting. So we need to discuss which behavior should be selected as a default. Currently, since this patch is only inteded to introduce bytecode intrisic system itself, I don't select the default behavior explicitly. I've opened the issue for that. https://bugs.webkit.org/show_bug.cgi?id=143996
> Ah, sorry for confusing. > > As described in ChangeLog, Oops! I only read the bug description, and not the ChangeLog :(. > So this patch itself doesn't change the current behavior. > And I'm trying to fix the current Array.from behavior in the separate from > this patch(implementing bytecode intrinsic system) > Array.from needs [[DefineOwnProperty]] and [[Set]]. So anyway, performing > both is needed. Got it.
(In reply to comment #12) > > Ah, sorry for confusing. > > > > As described in ChangeLog, > > Oops! I only read the bug description, and not the ChangeLog :(. > > > So this patch itself doesn't change the current behavior. > > And I'm trying to fix the current Array.from behavior in the separate from > > this patch(implementing bytecode intrinsic system) > > Array.from needs [[DefineOwnProperty]] and [[Set]]. So anyway, performing > > both is needed. > > Got it. :D. so closing this issue, OK?
Yup! Closed.