Open imiric opened 2 years ago
It surely results in unnecessary CDP calls and WS traffic to send the 28kB file over each time. ...we should look into it, and preferably call runtime.Evaluate() on the script only once per Frame.
We send the injected script once per frame because we cache it:
Then we return its handle, not its source code:
So, fortunately, we don't need to optimize it because we already cache the injected script per frame in an execution context (also belongs to a frame) 🥳
I ran a random test that evaluates on the same page with three different calls:
We injected the injected script only once and used its handle instead of sending it over the wire:
➜ k6b git:(add/query-all-unit-test) XK6_BROWSER_LOG=trace go test ./tests -run=TestElementHandleQueryAll -v
...
msg="sid:91E91FAD165D303DBA1B0FA1D57B1873 selector:#aul" category="Page:Query" elapsed="0 ms" goroutine=168
...
The script injection happens once here:
msg="-> {\"id\":22,\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\",\"method\":\"Runtime.evaluate\",\"params\":{\"expression\":\"(() =\\u003e {...class InjectedScript {...}" category="cdp:send" elapsed="0 ms" goroutine=43
time="2022-02-11T10:45:02+03:00" level=trace msg="<- {\"id\":22,\"result\":{\"result\":{\"type\":\"object\",\"className\":\"InjectedScript\",\"description\":\"InjectedScript\",\"objectId\":\"8165940339265855193.2.2\"}},\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\"}" category="cdp:recv" elapsed="1 ms" goroutine=42
Then querySelector
call happens through the injected script's handle (not its source code!):
msg="-> {\"id\":23,\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\",\"method\":\"Runtime.callFunctionOn\",\"params\":{\"functionDeclaration\":\"\\n\\t\\t(node, injected, selector) =\\u003e {\\n\\t\\t\\treturn injected.querySelector(selector, node || document, false);\\n\\t\\t}\\n\\t\\n//# sourceURL=__xk6_browser_evaluation_script__\\n\",\"arguments\":[{\"objectId\":\"8165940339265855193.2.1\"},{\"objectId\":\"8165940339265855193.2.2\"},{\"value\":{\"selector\":\"#aul\",\"parts\":[{\"name\":\"css\",\"body\":\"#aul\"}],\"capture\":null}}],\"userGesture\":true,\"awaitPromise\":true,\"executionContextId\":2}}" category="cdp:send" elapsed="0 ms" goroutine=43
msg="<- {\"id\":23,\"result\":{\"result\":{\"type\":\"object\",\"subtype\":\"node\",\"className\":\"HTMLUListElement\",\"description\":\"ul#aul\",\"objectId\":\"8165940339265855193.2.3\"}},\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\"}" category="cdp:recv" elapsed="1 ms" goroutine=42
The next call also happens through the injected script's handle. So we don't send the injected script twice over the wire.
msg="sid:91E91FAD165D303DBA1B0FA1D57B1873 stid:DB6B9C62692BA8DEF72C07F255B6C820 fid:DB6B9C62692BA8DEF72C07F255B6C820 ectxid:2 efurl:about:blank" category="ExecutionContext:getInjectedScript" elapsed="0 ms" goroutine=258
...
msg="-> {\"id\":27,\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\",\"method\":\"Runtime.callFunctionOn\",\"params\":{\"functionDeclaration\":\"...function QueryAll(scope = document, injected, selector) {\\n return injected.querySelectorAll(selector, scope);\\n}\\n\\n//# sourceURL=__xk6_browser_evaluation_script__\\n\",\"arguments\":[{\"objectId\":\"8165940339265855193.2.1\"},{\"objectId\":\"8165940339265855193.2.2\"},{\"value\":{\"selector\":\"li.ali\",\"parts\":[{\"name\":\"css\",\"body\":\"li.ali\"}],\"capture\":null}}],\"userGesture\":true,\"awaitPromise\":true,\"executionContextId\":2}}" category="cdp:send" elapsed="0 ms" goroutine=43
msg="<- {\"id\":27,\"result\":{\"result\":{\"type\":\"object\",\"subtype\":\"array\",\"className\":\"Array\",\"description\":\"Array(2)\",\"objectId\":\"8165940339265855193.2.8\"}},\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\"}" category="cdp:recv" elapsed="0 ms" goroutine=42
The final call also happens through the handle:
msg="fid:DB6B9C62692BA8DEF72C07F255B6C820 furl:\"about:blank\" sel:\"li.ali\"" category="Frame:QueryAll" elapsed="0 ms" goroutine=268
msg="fid:DB6B9C62692BA8DEF72C07F255B6C820 furl:\"about:blank\"" category="Frame:document" elapsed="0 ms" goroutine=268
msg="sid:91E91FAD165D303DBA1B0FA1D57B1873 stid:DB6B9C62692BA8DEF72C07F255B6C820 fid:DB6B9C62692BA8DEF72C07F255B6C820 ectxid:2 efurl:about:blank" category="ExecutionContext:getInjectedScript" elapsed="0 ms" goroutine=268
...
msg="-> {\"id\":30,\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\",\"method\":\"Runtime.callFunctionOn\",\"params\":{\"functionDeclaration\":\"...function QueryAll(scope = document, injected, selector) {\\n return injected.querySelectorAll(selector, scope);\\n}\\n\\n//# sourceURL=__xk6_browser_evaluation_script__\\n\",\"arguments\":[{\"objectId\":\"8165940339265855193.2.1\"},{\"objectId\":\"8165940339265855193.2.2\"},{\"value\":{\"selector\":\"li.ali\",\"parts\":[{\"name\":\"css\",\"body\":\"li.ali\"}],\"capture\":null}}],\"userGesture\":true,\"awaitPromise\":true,\"executionContextId\":2}}" category="cdp:send" elapsed="0 ms" goroutine=43
msg="<- {\"id\":30,\"result\":{\"result\":{\"type\":\"object\",\"subtype\":\"array\",\"className\":\"Array\",\"description\":\"Array(2)\",\"objectId\":\"8165940339265855193.2.12\"}},\"sessionId\":\"91E91FAD165D303DBA1B0FA1D57B1873\"}" category="cdp:recv" elapsed="0 ms" goroutine=42
Since it's an important detail, it's better to write a test for this.
@imiric WDYT about this? I think we can test this pretty quickly like this:
// ExecutionContext represents a JS execution context.
type ExecutionContext struct {
eval evalFunc
...
}
ec.eval := fakeEval(...)
ec.getInjectedScript(ctx) // check whether it's been cached
Hey, thanks for looking into this! I completely missed that this caching would ensure it's only sent once. I also confirmed it with a packet capture, where only the first call would send the full script, and subsequent calls would use its objectID
, e.g.:
"arguments":[{"objectId":"-4888431733462557698.2.13"},{"objectId":"-4888431733462557698.2.2"},{"value":{"selector":"li.ali","parts":[{"name":"css","body":"li.ali"}]
I'll remove the bug
and optimization
labels.
As for the test, sure, that sounds useful if it's a quick one and you don't mind. :thumbsup:
This issue turned out to be invalid but we decided to add a test.
Currently (4ae711e) the
injected_script.js
file is evaluated in the browser on each call of manyElementHandle
methods:checkHitTargetAt
checkElementState
dispatchEvent
fill
focus
offsetPosition
selectOption
selectText
waitForElementState
waitForSelector
OwnerFrame
Query
QueryAll
See: https://github.com/grafana/xk6-browser/blob/4ae711e4a63edec5c98e9a70fd6859dcd4f9574d/common/element_handle.go#L1414-L1426
Some of the unexported ones might be called several times per a JS method call.
I'm not sure if this results in a memory impact in the browser, but it surely results in unnecessary CDP calls and WS traffic to send the 28kB file over each time. I haven't checked if
cdproto/runtime
has any optimizations regarding this, but we should look into it, and preferably callruntime.Evaluate()
on the script only once perFrame
.The same applies for the wrapping functions defined inline in each method, they should be evaluated only once, but this shouldn't have as much impact as
injected_script.js
.