Open shwarcu opened 1 year ago
shwarcu
commented
1 year ago I'm trying to improve replaceIframes with memoization so that placeholder is not created multiple times for the same iframe. https://github.com/shwarcu/cypress/commit/c01ea25d65f3bc647036cb14ddd0cc4d2f64f382
but running Cypress from source on a windows machine seems to be nearly impossible, this stops me from actually testing my changes 😭
iframe dimensions probably could be memoized too?
lmiller1990
commented
1 year ago I am able to run Cypress from source on a windows machine. It's very slow to develop on windows, not sure if it's just my setup or something else.
In run mode (what you use on CI, like npx cypress run I don't think we take snapshots (since it's not really useful there). Taking a look at that code path might be of interest to you.
There is some work going on to improve perf specifically in Chrome around memory usage, so if it's just a memory issue, it's possible this will be addressed soon (during Q1).
If there is some low-hanging optimizations using memoization, that'd be great! What issue are you running into when running Cypress locally? You should be able to do
And that should be it - ready to go.
If you are happy to just hack something in without modifying Cypress from source, it might be possible to do so. I had a look around, I noticed a few things in packages/driver:
All queries have a snapshot flag:
// presses a key or clicks in the UI to continue
Commands.addQuery('pause', function pause (options: Partial<Cypress.Loggable> = {}) {
if (!config('isInteractive') && (!config('browser').isHeaded || config('exit'))) {
return _.identity
}
const log = options.log !== false && Cypress.log({
snapshot: true, // <========= here
autoEnd: false,
timeout: 0,
})
// ...
})I wonder if there is some way to override this using a custom command, that does the same thing without a snapshot. Or, can you override Cypress.log and change how that works? Cypress is available globally.
I also saw
I wonder if there is a way to override Cypress.action('cy:snapshot', name) somehow.
Alternatively, I wonder if you can intercept the cy:snapshot event: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress.ts#L643-L644
All of these are more or less hacks, but would let you do what you want without forking/requiring a patch to Cypress (which can be a longer process, since adding a feature requires more maintenance, docs, etc).
shwarcu
commented
1 year ago Thanks a lot for the message. I will tinker around in my spare time and will try to see if any memoization or disabling snapshots is going to improve performance 🤓
I managed to run Cypress locally on a MBP with M1. But on windows 10 I keep getting errors from postinstall script.
I have node v18.13.0 and yarn 1.22.19 so in theory I meet requirements
"engines": {
"node": ">=16.16.0",
"yarn": ">=1.17.3"
},
@packages/errors: '..' is not recognized as an internal or external command,
Weird, no idea - I'm not sure I can debug the windows development environment right now, though.
PRs that improve performance are always welcome -- I don't think we can commit to a specific "no snapshots for network requests" config option, Cypress already has so many ways to be configured. Any configuration changes need to be carefully considered, so it's more work than just writing the code.
Run mode doesn't capture snapshots (at all), you might be able to follow the code path and see if you can replicate that for open mode, if that's something you still want to pursue as a personal challenge. I'd be interested in how significant the perf gains are!
shwarcu
commented
1 year ago @lmiller1990 Instead of looking for ways to disable snapshots I kept lurking around this function replaceIframes and it bothers me that it will continuously create new iframe placeholder for the same iframe over and over when there are requests being made in the background by application under test.
I have made a silly attempt to cache iframe placeholder https://github.com/shwarcu/cypress/pull/5/files
but creating a snapshot works only for the first time, then no snapshots are created and run time of replaceIframes is not even measured after first two calls.
In the PR I have attached video showing how it looks like on my example app. Do you have any idea what I might have missed that broke snapshots?
lmiller1990
commented
1 year ago continuously create new iframe placeholder
Just to clarify, are you saying it's creating a new snapshot for every request? I think we've talked about this in other issues before, one proposal that's been floated was to use a MutationObserver (or something similar) to compare the snapshots, and skip creating a new snapshot if nothing changes in the DOM.
I don't see the problem in your code immediately, this part of the app is pretty complex, so I'd need to dig into it a little more. I left a comment, take a look at that.
The way I normally debug this is just putting lots of debugger statements and stepping through, building a mental model of what's actually going on.
It's also very easy to introduce a memory leak in this part of the app, I'm pretty sure we already have one, so keep that in mind if you are experimenting here. I cannot guarantee we have the bandwidth to accept a PR here, but any exploration and useful information is always good to have - maybe there is some low hanging optimizations to be made (this sure would be nice).
shwarcu
commented
1 year ago @lmiller1990 I didn't mean 'crating a new snapshot for every request' this time, I was talking about inner part of this process. Let me explain 🧠 Creating a snapshot for every request is one (let's say) issue. Another thing is that when we have an iframe on the page then during snapshot creation Cypress is creating a placeholder for that iframe and sets some basic styling to it. The problem (I think) that exists within this mechanism is that we don't verify if there is any change in the iframe properties that would justify creating another placeholder for an iframe.
In a scenario where there are many requests performed, and page contains some juicy iframe/iframes, then creating iframe placeholders every second or x milliseconds becomes a noticeable performance issue.
In my understanding, if we have the same iframe and when it doesn't change size or some other css properties, we could skip creating another iframe placeholder and just use previously created one. That's the optimisation I was trying to implement. The idea was to take some properties of an iframe (id, styles and so on), create some sort of 'hash' out of it and store it in a map with corresponding iframe placeholder. So that next time when we create a snapshot, and there is an iframe with the same properties we could re-use previously created iframe placeholder.
It's difficult to have more insightful understanding of that code just by reading it because in a lot of places it's not using types. Whenever I take a look at some const or function argument it turns out to be any and I have to walk around and find out about that object by looking at places where it's used. It's quite frustrating because I really wanted to find ways for optimisation and contribute to the project :< Maybe I will give it a try with debugger and see how the code works during runtime, but to be honest I'm quite discouraged at the moment 😅 Especially when you say that even if I open PR it might not be considered
lmiller1990
commented
1 year ago in a lot of places it's not using types
Cypress was originally plain JS, but if you want to make a PR improving types, that is definitely something that can be accepted easily! It's hard to figure out the types sometimes, which is why no-one has done it yet.
but to be honest I'm quite discouraged at the moment
Is this due to complexity or something else? Sorry if I said something to discourage you.
Especially when you say that even if I open PR it might not be considered
We can definitely consider the PR, we will just need to make sure there is no intended side effect, like excessive browser memory usage, etc. I re-reviewed your code and I see this is actually looks like it's pretty close to working, so it's probably a reasonable change, if we can figure out the problem.
Just to clarify, this change you are working on is unrelated to the original issue, right? I think this is why I was confused - this doesn't look like it has anything to do with network requests, does it?
You do
$placeholder = iframePlaceholders.get(key);The only thing I can think of is that this is returning undefined :thinking: everything else looks fine. I don't see any other obvious problem. I am not sure I can pull your code and debug it this week, I might be able to put some time in next week.
If you figure it out, please let me know and I can run our full test suite against your PR, that will verify if anything else got broken.
emilyrohrbough
commented
1 year ago @alexsch01 We are currently working on providing a mechanism to disable logging network requests for cy.intercept() in #7362. When logs are disabled for comments, we do not take snapshots. Do you feel this would alleviate some of the performance issues you are experiencing? or are you receiving so many network requests it's just the logging of these in the Command Log that is impacting performance?
alexsch01
commented
1 year ago @emilyrohrbough Hey can you send me my comments in the issue(s) where I was writing about performance issues? Very appreciated.
What would you like?
I would like to be able to disable DOM snapshots for network requests. Or even disable DOM snapshots for specific network requests by specifying domain or pathname. In the case I am working with I have a lot of network requests that cause Cypress to take enormous amount of snapshots, each snapshot taking takes ~56 milliseconds and causing performance problems. Significant part of taking snapshot is execution of function
replaceIframes-45.36 msin my case.Big picture - almost all "purple" blocks are event emitter calls that later down trigger snapshots
Closer view at some of such calls
replaceIframesIt would be great to have available option in Cypress config that would allow setting following behaviours for snapshots
Why is this needed?
I would like to improve performance when running cypress open. Also I would like to prevent browser tab from constantly crashing when there are too many snapshots.
Other
I know that option
numTestsKeptInMemoryexists but it don't want to disable snapshots completely or hold last 10 snapshots - they will be quickly overwritten by requests that are fired by application. I would like to keep DOM snapshots but skip making them for network requests.