Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] Improve Ajax Profiling Performance (javascript)#20197
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| varrenderAjaxRequests=function() { | ||
| var requestCounter=document.querySelectorAll('.sf-toolbar-ajax-requests'); | ||
| if (!requestCounter.length) { | ||
| var requestCounter=document.querySelectorAll('.sf-toolbar-ajax-requests')[0]; |
Member
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.
if you want only one,usedocument.querySelector instead
| if (!requestCounter) { | ||
| return; | ||
| } | ||
| requestCounter.textContent=requestStack.length; |
Member
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.
this DOM updates should be doneafter the retrieval of other needed nodes, to group read and writes
ContributorAuthor
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 think the readability > that the performance gain by moving all the writes in the same place. But will re-arrange if you still disagree.
| var row=document.createElement('tr'); | ||
| rows.insertBefore(row,rows.firstChild); | ||
| var infoSpan=document.querySelectorAll(".sf-toolbar-ajax-info")[0]; |
Member
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.
querySelector should be used here
| durationCell.textContent='-'; | ||
| } | ||
| row.appendChild(durationCell); | ||
| var tbody=document.querySelectorAll('.sf-toolbar-ajax-request-list')[0]; |
Member
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.
should bequerySelector
| } | ||
| {%if excluded_ajax_paths is defined%} | ||
| if (window.fetch&&window.fetch.polyfill===undefined) { |
Member
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.
please keep the indentation inside the Twigif. It makes the template much more readable
ContributorAuthor
patrick-mcdougle commentedOct 23, 2016
@stof if you get a chance, could you review again? Merge maybe? |
ContributorAuthor
patrick-mcdougle commentedOct 31, 2016
@stof any chance this can get on the 3.2 train? |
ContributorAuthor
patrick-mcdougle commentedNov 20, 2016
ContributorAuthor
patrick-mcdougle commentedDec 11, 2016
Member
fabpot commentedDec 13, 2016
Thank you@patrick-mcdougle. |
fabpot added a commit that referenced this pull requestDec 13, 2016
… (javascript) (patrick-mcdougle)This PR was squashed before being merged into the 3.3-dev branch (closes#20197).Discussion----------[WebProfilerBundle] Improve Ajax Profiling Performance (javascript)| Q | A || --- | --- || Branch? | master || Bug fix? | kinda (is bad performance a bug?) || New feature? | kinda (is increased performance a feature?) || BC breaks? | no (unless performance is a BC break) || Deprecations? | no || Tests pass? | do we have JS tests? || Fixed tickets |#20155 || License | MIT || Doc PR | n/a |The old version of this JS re-rendered the entire list of ajax calls which was causing some performance issues when people had high numbers of ajax requests (increasingly common as people create SPAs.This PR changes the behavior of the ajax profiler to be more smart about the DOM manipulations it makes. Instead of re-rendering the entire list, on any AJAX requests/responses, it instead adds the row to the profiler when an AJAX request is made, adding the DOM node as a property of the request on the requestStack. When the AJAX response comes back, it updates the existing DOM node instead of re-creating it (and all of the others).I've tested this on my machine using a modern version of chrome. I don't think I'm doing anything fancy, so I think the likelihood that I broke something is minimal.I've left a couple of the commits separate, because they represent distinct ideas. The first commit is just some consistency/cleanup. The second commit is the meat of the work. Its diff is basically useless since I've added two new functions and modified one function heavily. I tried to make the diff as easy to read as possible, but it's still pretty rough. The third commit removes some functions/calls that I don't think need to be there now that this is re-written, but I wanted to leave them in a separate commit for ease of revert if they are indeed needed.Commits-------65e391c Replace occurances of querySelectorAll with querySelectorfddff26 Put back the indentation9942edd Remove unnecessary method calls/definitions2c053ee Rewrite ajax profiling for performanceda621c9 Fix indentation & JS Cleanup
Merged
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull requestMar 25, 2018
…ormance (javascript) (patrick-mcdougle)This PR was squashed before being merged into the 3.3-dev branch (closessymfony#20197).Discussion----------[WebProfilerBundle] Improve Ajax Profiling Performance (javascript)| Q | A || --- | --- || Branch? | master || Bug fix? | kinda (is bad performance a bug?) || New feature? | kinda (is increased performance a feature?) || BC breaks? | no (unless performance is a BC break) || Deprecations? | no || Tests pass? | do we have JS tests? || Fixed tickets |symfony#20155 || License | MIT || Doc PR | n/a |The old version of this JS re-rendered the entire list of ajax calls which was causing some performance issues when people had high numbers of ajax requests (increasingly common as people create SPAs.This PR changes the behavior of the ajax profiler to be more smart about the DOM manipulations it makes. Instead of re-rendering the entire list, on any AJAX requests/responses, it instead adds the row to the profiler when an AJAX request is made, adding the DOM node as a property of the request on the requestStack. When the AJAX response comes back, it updates the existing DOM node instead of re-creating it (and all of the others).I've tested this on my machine using a modern version of chrome. I don't think I'm doing anything fancy, so I think the likelihood that I broke something is minimal.I've left a couple of the commits separate, because they represent distinct ideas. The first commit is just some consistency/cleanup. The second commit is the meat of the work. Its diff is basically useless since I've added two new functions and modified one function heavily. I tried to make the diff as easy to read as possible, but it's still pretty rough. The third commit removes some functions/calls that I don't think need to be there now that this is re-written, but I wanted to leave them in a separate commit for ease of revert if they are indeed needed.Commits-------65e391c Replace occurances of querySelectorAll with querySelectorfddff26 Put back the indentation9942edd Remove unnecessary method calls/definitions2c053ee Rewrite ajax profiling for performanceda621c9 Fix indentation & JS Cleanup
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The old version of this JS re-rendered the entire list of ajax calls which was causing some performance issues when people had high numbers of ajax requests (increasingly common as people create SPAs.
This PR changes the behavior of the ajax profiler to be more smart about the DOM manipulations it makes. Instead of re-rendering the entire list, on any AJAX requests/responses, it instead adds the row to the profiler when an AJAX request is made, adding the DOM node as a property of the request on the requestStack. When the AJAX response comes back, it updates the existing DOM node instead of re-creating it (and all of the others).
I've tested this on my machine using a modern version of chrome. I don't think I'm doing anything fancy, so I think the likelihood that I broke something is minimal.
I've left a couple of the commits separate, because they represent distinct ideas. The first commit is just some consistency/cleanup. The second commit is the meat of the work. Its diff is basically useless since I've added two new functions and modified one function heavily. I tried to make the diff as easy to read as possible, but it's still pretty rough. The third commit removes some functions/calls that I don't think need to be there now that this is re-written, but I wanted to leave them in a separate commit for ease of revert if they are indeed needed.