Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update link ref handling to clean up previous listeners #7943

Merged
merged 4 commits into from
Jul 14, 2019
Merged

Update link ref handling to clean up previous listeners #7943

merged 4 commits into from
Jul 14, 2019

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Jul 12, 2019

This makes sure the previous listeners are cleaned up before overwriting the cleanup function.

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 14.2s 13.8s -382ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB ✓
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ✓
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 222 B 222 B ✓
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 301 B 301 B ✓
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Base Rendered Size 1.36 kB 1.36 kB ✓
Build Dir Size 757 kB 756 kB -314 B
Click to expand serverless stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 14.5s 14.9s ⚠️ +384ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB -1 B
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ⚠️ +1 B
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 224 B 222 B -2 B
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 300 B 301 B ⚠️ +1 B
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Serverless pages/link Size 338 kB 338 kB -98 B
Serverless pages/link gzip Size 86.8 kB 86.7 kB -28 B
Serverless pages/index Size Error getting size Error getting size ✓
Serverless pages/index gzip Size Error getting size Error getting size ✓
Serverless pages/_error Size 328 kB 328 kB ✓
Serverless pages/_error gzip Size 84 kB 84 kB -1 B
Serverless pages/routerDirect Size 329 kB 329 kB ✓
Serverless pages/routerDirect gzip Size 84.3 kB 84.3 kB ✓
Serverless pages/withRouter Size 329 kB 329 kB ✓
Serverless pages/withRouter gzip Size 84.3 kB 84.3 kB -2 B
Build Dir Size 1.94 MB 1.94 MB -314 B

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 12.7s 12.4s -255ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB ✓
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ✓
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 222 B 222 B ✓
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 301 B 301 B ✓
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Base Rendered Size 1.36 kB 1.36 kB ✓
Build Dir Size 757 kB 756 kB -314 B
Click to expand serverless stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 13.7s 13.6s -55ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB ✓
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ✓
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 222 B 222 B ✓
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 301 B 301 B ✓
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Serverless pages/link Size 338 kB 338 kB -98 B
Serverless pages/link gzip Size 86.8 kB 86.7 kB -26 B
Serverless pages/index Size Error getting size Error getting size ✓
Serverless pages/index gzip Size Error getting size Error getting size ✓
Serverless pages/_error Size 328 kB 328 kB ✓
Serverless pages/_error gzip Size 84 kB 84 kB ✓
Serverless pages/routerDirect Size 329 kB 329 kB ✓
Serverless pages/routerDirect gzip Size 84.3 kB 84.3 kB ✓
Serverless pages/withRouter Size 329 kB 329 kB ✓
Serverless pages/withRouter gzip Size 84.3 kB 84.3 kB ⚠️ +1 B
Build Dir Size 1.94 MB 1.94 MB -314 B

@brad-decker
Copy link

@ijjk if you’d like me to test this out on the prod site I reported the issue on in the react thread I can.

@timneutkens
Copy link
Member

@brad-decker that'd be great!

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 14.5s 14.2s -328ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB ✓
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ✓
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 222 B 222 B ✓
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 301 B 301 B ✓
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Base Rendered Size 1.36 kB 1.36 kB ✓
Build Dir Size 757 kB 756 kB -314 B
Click to expand serverless stats
zeit/next.js canary ijjk/next.js fix/link-ref-handling Change
Build Duration 15.3s 15.1s -255ms
node_modules Size 45.4 MB 45.4 MB -93 B
Total Bundle (main, webpack, commons) Size 210 kB 210 kB ✓
Total Bundle (main, webpack, commons) gzip Size 68.8 kB 68.8 kB ✓
Client _app Size 2.39 kB 2.39 kB ✓
Client _app gzip Size 1.08 kB 1.08 kB ✓
Client _error Size 8.45 kB 8.45 kB ✓
Client _error gzip Size 3.24 kB 3.24 kB ✓
Client pages/index Size 296 B 296 B ✓
Client pages/index gzip Size 222 B 222 B ✓
Client pages/link Size 4.19 kB 4.14 kB -55 B
Client pages/link gzip Size 1.82 kB 1.82 kB -4 B
Client pages/routerDirect Size 423 B 423 B ✓
Client pages/routerDirect gzip Size 306 B 306 B ✓
Client pages/withRouter Size 435 B 435 B ✓
Client pages/withRouter gzip Size 301 B 301 B ✓
Client main Size 23.7 kB 23.7 kB ✓
Client main gzip Size 7.81 kB 7.81 kB ✓
Client commons Size 184 kB 184 kB ✓
Client commons gzip Size 59.7 kB 59.7 kB ✓
Client webpack Size 1.49 kB 1.49 kB ✓
Client webpack gzip Size 770 B 770 B ✓
Serverless pages/link Size 338 kB 338 kB -98 B
Serverless pages/link gzip Size 86.8 kB 86.7 kB -26 B
Serverless pages/index Size Error getting size Error getting size ✓
Serverless pages/index gzip Size Error getting size Error getting size ✓
Serverless pages/_error Size 328 kB 328 kB ✓
Serverless pages/_error gzip Size 84 kB 84 kB ✓
Serverless pages/routerDirect Size 329 kB 329 kB ✓
Serverless pages/routerDirect gzip Size 84.3 kB 84.3 kB -1 B
Serverless pages/withRouter Size 329 kB 329 kB ✓
Serverless pages/withRouter gzip Size 84.3 kB 84.3 kB ⚠️ +1 B
Build Dir Size 1.94 MB 1.94 MB -314 B

@brad-decker
Copy link

Test:

  1. in dev mode preload two pages.
  2. hard refresh and trigger GC to get to a clean start
  3. start performance profiler
  4. run for 20 seconds flipping between the two pages
  5. wait for 5 seconds then end performance monitor
  6. repeat with this branch in my app using the file: pointer as per the contributor file.

before

image

after

image

Analysis

While I am still able to push my dom nodes to 20,000+ with this fix in, GC eventually returns the memory to me. At the end of the first test, there were still 27k dom nodes in memory. At the end of the second test, I had dropped down to 9k. Our pages need to be optimized for dom nodes, one of the two pages has 6k due to large custom SVGs that haven't been optimized yet. So 9k is reasonable :P

Conclusion

This is at least a step in the right direction. Our application has other unrelated memory leaks, but if I find anything else while we unravel these I can report them to you in any way you wish. Thank you both for the quick turnaround @ijjk and @timneutkens

@ijjk ijjk merged commit 4adef73 into vercel:canary Jul 14, 2019
@ijjk ijjk deleted the fix/link-ref-handling branch July 14, 2019 17:23
@Timer Timer added this to the 9.0.2 milestone Jul 15, 2019
@brad-decker brad-decker mentioned this pull request Jul 15, 2019
Closed
@vercel vercel locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@timneutkens timneutkens timneutkens approved these changes

@brad-decker brad-decker brad-decker approved these changes

@lfades lfades Awaiting requested review from lfades

@Timer Timer Awaiting requested review from Timer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.0.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ijjk @brad-decker @timneutkens @Timer