[WebProfilerBundle] Improve Ajax Profiling Performance (javascript) #20197
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
stof
reviewed
Oct 11, 2016
| var renderAjaxRequests = function() { | ||
| var requestCounter = document.querySelectorAll('.sf-toolbar-ajax-requests'); | ||
| if (!requestCounter.length) { | ||
| var requestCounter = document.querySelectorAll('.sf-toolbar-ajax-requests')[0]; |
There was a problem hiding this comment.
if you want only one,use document.querySelector instead
| return; | ||
| } | ||
| requestCounter.textContent = requestStack.length; |
There was a problem hiding this comment.
this DOM updates should be done after the retrieval of other needed nodes, to group read and writes
There was a problem hiding this comment.
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]; |
| durationCell.textContent = '-'; | ||
| } | ||
| row.appendChild(durationCell); | ||
| var tbody = document.querySelectorAll('.sf-toolbar-ajax-request-list')[0]; |
| @@ -237,91 +241,86 @@ | |||
| } | |||
|
|
|||
| {% if excluded_ajax_paths is defined %} | |||
| if (window.fetch && window.fetch.polyfill === undefined) { | |||
There was a problem hiding this comment.
please keep the indentation inside the Twig if. It makes the template much more readable
|
@stof if you get a chance, could you review again? Merge maybe? |
|
@stof any chance this can get on the 3.2 train? |
|
@javiereguiluz @stof @fabpot Bump |
|
Thank you @patrick-mcdougle. |
fabpot
added a commit
that referenced
this pull request
Dec 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 querySelector fddff26 Put back the indentation 9942edd Remove unnecessary method calls/definitions 2c053ee Rewrite ajax profiling for performance da621c9 Fix indentation & JS Cleanup
Merged
ostrolucky
pushed a commit
to ostrolucky/symfony
that referenced
this pull request
Mar 25, 2018
…ormance (javascript) (patrick-mcdougle) This PR was squashed before being merged into the 3.3-dev branch (closes symfony#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 querySelector fddff26 Put back the indentation 9942edd Remove unnecessary method calls/definitions 2c053ee Rewrite ajax profiling for performance da621c9 Fix indentation & JS Cleanup
Sign up for free
to 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.