-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(learn): add article on tuning memory #7639
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (1)
- packages/i18n/locales/en.json: Language not supported
jsumners-nr
left a comment
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.
Thank you for taking this on and so quickly. I have some feedback, but overall it's looking good.
I'd also like to see @mcollina's feedback on it.
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Outdated
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Show resolved
Hide resolved
apps/site/pages/en/learn/diagnostics/memory/understanding-and-tuning-memory.md
Show resolved
Hide resolved
jsumners-nr
left a comment
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.
I'm finding it difficult to re-review the things I had noted originally because they have been marked as resolved and no longer show up inline in the change set. But I am getting a distinct impression that a predictive text generator was used to address the concerns I raised. I would like to see very clear scenarios in which each setting would be useful, and a discussion about what one can expect when making a change with the setting.
|
I've addressed the conversations in commit 24bcca2. If the examples I included don’t meet your expectations, I’m open to hearing suggestions for improvement. That said, I find it a bit insulting to have my writing likened to a “predictive text generator.” |
The commit diff helps, thank you.
The thing is, I'm not seeing many actual examples. Lines 115 and 119 in the linked change set are highlighting some specific cases, but the others are mainly wordier rephrasings of the original blocks. Like in line 103 of that change set: what constitutes "high throughput" and what is the thing that is "noticeable"? I think the document should assume the reader has a basic grasp on memory concepts, but is by no means an expert and is trying to expand their knowledge through this doc. I think such a reader would be looking for concrete examples along the lines of "in an application that does A, with B requests per second, and generates objects that look like
All I can go on is the evidence before me. What I see is as I described: moderate rephrasings of the reviewed blocks of text. That's what I would expect from such a tool. |
|
Thanks for the additional feedback, e1e3316 should address your review, along with one minor issues that occur when using non-VSCode editors (Seriously, why do half of my editors use fancy punctuation?).
I'm always rewording things, so that's unrelated to your reviews. I'm sorry if they didn't address your concerns |
mcollina
left a comment
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.
lgtm
|
Lighthouse Results
|
Description
This PR adds an article on understanding and tuning Memory in Node.js. It discusses the V8 memory types (Stack and Heap), along with Old vs New space.
CC @nodejs/diagnostics
Validation
After building the package, the article should exist at
/learn/diagnostics/memory/understanding-and-tuning-memoryRelated Issues
Fixes #7637
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.