-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: #1881 fixed memory leak for validation #1892
Conversation
@lukasz-kuzynski-11sigma Neat! @radzserg I'd be 👍 to get this merged as this indeed fixes the issue. Then, you'll have to tell me if you think there's any value in me finalizing #1890 (FWIW, I think there is some value in untangling/decoupling the generation of the validation schema from the actual execution of this validation. However, it'll certainly end as a big PR with 30+ modified files and lots of touched tests, so it may be tiresome to review for you guys...) |
@lukasz-kuzynski-11sigma How would you feel about adding some unit test that would run many validations in a loop, monitor the heap usage and fail if it grows above a certain percentage? |
I compared your solutions using the original performance test from @nulltoken branch on 100K loop (input + output validation) and performances are pretty the same. |
And yes let's add some tests to ensure we don't have a memory leak. Then this one will be ready for a merge. |
Cool with me 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to figure out why test fails on 10K loop
Please do not merge it just yet. Once I've changed the spec to use the ones from the fixtures it turns out we're still leaking. I'll tackle it a little bit later today/tomorrow morning. |
@lukasz-kuzynski-11sigma How are things going on your side? Have you managed to find out where does the additional leak come from? |
Extra note. I've tested entire setup outside of tests scope just by running prism standalone to prevent potential issues with jest and easier profiling. Then I ran 50 POST requests, profiled memory with devtools and repeat the process several time. After the fix there are no more serious memory leaks. What I've observed is that prism memory usage oscillates around certain value (in my case it was 49-50 MB) but does not grow significantly anymore. Once again thank You @nulltoken for reporting that and help. Your feedback is invaluable <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 👍
@lukasz-kuzynski-11sigma Thanks a lot for the fix! ❤️ Just a quickie: In the paragraph above you mention "there are no more serious memory leaks" and "does not grow significantly anymore.". Just to make sure I fully understand you, can you confirm that, once the fix is applied, whatever the "traffic", the memory keeps on oscillating between fixed lower and upper bounds? Sorry for asking, just willing to make sure those serious/significantly qualifiers do not mean that the leak is now mostly fixed (but keep at leaking at a much much lower rate). |
@nulltoken Yes I can confirm that after the fix the memory usage is normal and oscillates around certain values based on the size of your specification. Sometimes it takes few seconds for GC to properly remove everything but definitely the memory usage does not grow significantly. In my case it was around 51MB ( Also I'm 100% certain is that there are no more leaks regard validation :) The minor leaks that I've spotted in profiler are truly minor and might be also related to logger, node.js internals or just typescript transpiler. Either way we're going to release the fix and once the problem with memory leaks strikes back then we go deeper. PS. No need to sorry. I deeply appreciate your help :) |
@lukasz-kuzynski-11sigma Awesome! Please ping me as soon as it's released, I've got quite the configuration to put it to the test 😉 |
@nulltoken Hello. Newest version is released. Please let us know if everything is working fine for You :) |
@lukasz-kuzynski-11sigma Sorry, I got sidetracked and forgot to provide you with some feedback. We've ran it for several days in staging environment without any issue. It's been promoted to Production since. Great job and many thanks! |
I'm more than happy to hear that! @nulltoken |
Addresses #1881
Summary
Replace this with something that explains what this PR is for and why it matters.
Checklist
Screenshots
Additional context
We've been recreating schema for params everytime, therefore any we couldn't effectively cache validation function created by ajv.