Bug 1358414 is blocked because of performance issues in the netmonitor.
Over time we piled up a significant complexity in the DOM tree used to build netmonitor UI. And to prevent unnecessary reflows/repaints, hacks have been used in the CSS rules. But these hacks, like the removal of display: table-row on the table rows... are not making any sense. Whereas at the same time, if you remove such hack, suddently, the whole request table repaints everytime a new request is added to the list.

Bug 1527333 highlights another suspicious rule: position:absolute on the <table> element. This is is yet another hacky way to implement a scrollable <table>.

We can probably iterate over netmonitor DOM and CSS in order to simplify it, remove any DOM element that isn't strictly required (we are having many "wrappers" elements) and remove CSS rules that have to impact on layout, nor performance.

(In reply to Alexandre Poirot [:ochameau] from comment #0)

Bug 1527333 highlights another suspicious rule: position:absolute on the <table> element.

Correction: this problematic position:absolute styling is actually on the display:table-row-group element. This makes it effectively display:block and breaks the table-part parenting. See bug 1358414 comment 34 for why this causes trouble (particularly when you try to put display:table-row elements inside of this not-actually-a-table-row-group element.)

If we want to keep the position:absolute hack for the time being, then one possible incremental way forward here would be to simply formalize the current structure and make anonymous things non-anonymous. In particular (assuming that we do really want table formatting here), we could do the following, which I think would avoid the table-row perf issue:

• Drop the table-row-group styling from requests-list-contents (it gets stomped on by position:absolute, so it's useless/misleading)
• Give "requests-list-contents" a <div style="display:table"><div style="display:table-row-group> wrapper around its request-list-item children (and insert any new request-list-item elements inside of this legitimate table-row-group).

This should produce the same box tree that we already get when you add table-row styling, except now the anonymous table parts will be be non-anonymous & addressable (i.e. you can actually insert the new request-list-item children inside the real table-row-group, rather than just throwing them into a block and letting the frame constructor take care of things).

This way, when you insert new request-list-item children (as display:table-row elements), they will see that they are inserting inside of an actual table-row-group element and that their world makes sense. This avoids the frametree reconstruction discussed in bug 1358414 comment 34.

This is used to do a scrollable table.
So that we can re-introduce display:table-row; on rows and remove any position:absolute on table elements.

Unfortunately, this patch regress the performance as the whole table is repainted when a new row is added.

I'm also using display: table-row, just to simplify things and have the header be just a regular row.
This change is just to ensure the special things around sticky table header isn't a source of complexity.

Depends on D20383

(In reply to Daniel Holbert [:dholbert] from comment #2)

If we want to keep the position:absolute hack for the time being,

Let's assume we want nothing. Nothing except a functional netmonitor without unexpected repaint/reflow.
We do not care about using any particular CSS trick, nor any particular DOM Element.
But we can't just rewrite the netmonitor from scratch and we have to to iterate from the existing code.
That's what makes everything complex.

The usage of position:absolute is related to the overflow: auto.
It intends to implement a scrollable table.

Using overflow:auto on a <table> doesn't work. It doesn't produce a scrollable table, instead, the table expands to the full height of all its rows.

Here is an example:
data:text/html,<table id="table" style="border:1px solid black; width: 200px; height: 100px; overflow: auto;"><tr><td style="font-size: 100px">foo</td></tr></table><script>setInterval(()=>{var table=document.getElementById("table"); table.firstChild.appendChild(table.firstChild.firstChild.cloneNode(true));}, 1000);</script>

This <table> will just expand as we add rows.
Instead, we should let the table have no style and expand, and instead have a container which has a fixed size and have the overflow:auto. We end up using position:absolute for the "fixed size". I think that's the only reason. We could probably get rid of it by using flex down to this element.

Here is another example to demonstrate that it works fine with a simple container:
data:text/html,<div style="border:1px solid black; width: 200px; height: 100px; overflow: auto;"><table id="table"><tr><td style="font-size: 100px">foo</td></tr></table><script>setInterval(()=>{var table=document.getElementById("table"); table.firstChild.appendChild(table.firstChild.firstChild.cloneNode(true));}, 1000);</script>
(And there is no unexpected repaint on new rows [verified via nglayout.debug.paint_flashing])

then one possible incremental way forward here would be to simply formalize the current structure and make anonymous things non-anonymous. In particular (assuming that we do really want table formatting here), we could do the following, which I think would avoid the table-row perf issue:

• Drop the table-row-group styling from requests-list-contents (it gets stomped on by position:absolute, so it's useless/misleading)
• Give "requests-list-contents" a <div style="display:table"><div style="display:table-row-group> wrapper around its request-list-item children (and insert any new request-list-item elements inside of this legitimate table-row-group).

In these two patches I'm doing something somewhat similar and try to cleanup the table even more:

• requests-list-table becomes a <div> with the position:absolute and overflow:auto
• requests-list-contents becomes a <table> via display: table
• requests-list-headers is now a direct child of the <table> and uses display: table-row
• requests-list-item uses display: table-row

The DOM tree now looks like this:
<div class="requests-list-table" style="position: absolute; overflow: auto; width: xxx px; height: xxx px;">
<table class="requests-list-contents">
<tr class="requests-list-headers">
<td class="requests-list-column" />...
<tr class="requests-list-item">
<td class="requests-list-column" />...

div.requests-list-table is having its width and height manually defined by JS. For some reason in the past, it helped prevent unnecessary repaint. But we can probably get rid of it.

This way, when you insert new request-list-item children (as display:table-row elements), they will see that they are inserting inside of an actual table-row-group element and that their world makes sense. This avoids the frametree reconstruction discussed in bug 1358414 comment 34.

In this setup, I'm not having a table-row-group. Instead, the rows are added directly to the <table>, is that an issue?
(I quickly tried to have an intermediate table row group, but that did not helped getting rid of repaint when new lines are added)

Daniel, Do you think you could be able to know why or what causes the full table to repaint when adding a new row? I mean... with these two patches applied, which hopefully address the issue around position:absolute+display:table-row correctly.

I'm waiting for DAMP results, but locally the results looks good via the paint flashing preference.
Not getting any unexpected repaint required fixing many things at once:

• the position:absolute
• using a table-row-group
• parenting all elements correctly without any unexpected in-between elements (i.e. table-row are direct childs of row-group, row-group is a direct child of table, same for header-group, ...)

Sorry, caught up on my bugmail after our IRC discussion and just saw your question here. :)

(In reply to Alexandre Poirot [:ochameau] from comment #5)

If we want to keep the position:absolute hack for the time being,
The usage of position:absolute is related to the overflow: auto.
It intends to implement a scrollable table.

Using overflow:auto on a <table> doesn't work. It doesn't produce a scrollable table, instead, the table expands to the full height of all its rows.

Do you really need it to be position:absolute, though? It sounds like maybe you just need overflow:auto which works on blocks though apparently doesn't work on tables. And position:absolute is a complex thing which had the side effect of making this element into a block and thereby making overflow do what you expected -- but really maybe the display:block is all that you need (with absolute being unnecessary) for the scrollable element?

Daniel, Do you think you could be able to know why or what causes the full table to repaint when adding a new row?

I think we sorted this out over IRC.

(In reply to Alexandre Poirot [:ochameau] from comment #5)

(In reply to Daniel Holbert [:dholbert] from comment #2)

If we want to keep the position:absolute hack for the time being,

Let's assume we want nothing. Nothing except a functional netmonitor without unexpected repaint/reflow.

It make sense to remove position:absolute since we are also removing that in our "resizing" patch (in bug 1358414).

Honza

It looks like this patch doesn't have any impact on performance, which was exactly what I was looking for, surprisingly ;)

But at least it simplifies the existing DOM and CSS of the netmonitor (we can probably simplify it even more).
I've not looked at the resizing patch, but my hope is that if we rebase against this patch, it should introduces less differences and may be no longer impact the performance in a negative way??
If it still does we may try to find way to land it incrementally so that we can find what particular modification is regressing.
Note that you don't have to push to DAMP to have a first idea. Toggle nglayout.debug.paint_flashing_chrome, and see if you get repaints when a new request is added. Only the new line and the waterfall should be painted. If the new lines is added out of the viewport, only the waterfall should be repainted.

Here is DAMP results:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=eb955be47362d2f5f131afba7b37b979e2a5c55f&newProject=try&newRevision=be632846fa9bcab583985e9bf912907addee3e14&originalSignature=1759151&newSignature=1759151&framework=12

Thank you everyone for taking a look at this. We have done some structure modifications already along the way (as part of bug 1358414) so I want to mention it here, since we would have to merge all related changes:

• removed the header <div.requests-list-headers> from <div.request-list-empty-notice> because there is no need to display headers when there are no requests
• changed <div.requests-list-headers> from display: table-header-group; to display: table-row;
• moved <div.requests-list-headers-wrapper> (made it display:table-header-group) ABOVE the <div.requests-list-contents> (table-row-group) while keeping position: sticky for <div.requests-list-headers-wrapper>
• removed position:absolute from <div.requests-list-contents>
• added table-layout: fixed to <div.requests-list-table> and removed position: relative
• added AGAIN to <div.request-list-item> display: table-row; and removed position: relative (although I'm not sure here)
• renamed <div.requests-list-wrapper> to <div.requests-list-scroll> because that's the one that takes care of the scrolling (we couldn't get rid of it because it is NOW used for scroll)
• I have done my best to simplify the CSS although I am no expert :-) and you can certainly give more insight

Please, take a look at the latest patch in bug 1358414 so you can see the exact DOM structure there. It would really help if we could more or less go the same direction regarding structural changes that you do in this bug and that we do in the project for resize-columns. It could save some work. Thanks so much!

Lenka

(In reply to Lenka Pelechova from comment #10)

Thank you everyone for taking a look at this. We have done some structure modifications already along the way (as part of bug 1358414) so I want to mention it here, since we would have to merge all related changes:

• removed the header <div.requests-list-headers> from <div.request-list-empty-notice> because there is no need to display headers when there are no requests

Is this a change we can land individually? If yes, we should!

• changed <div.requests-list-headers> from display: table-header-group; to display: table-row;

Do you have any particular reason to do it? In my patch I'm keeping table-header-group as it doesn't seem to have any negative impact on performance.

• moved <div.requests-list-headers-wrapper> (made it display:table-header-group) ABOVE the <div.requests-list-contents> (table-row-group) while keeping position: sticky for <div.requests-list-headers-wrapper>

I'm doing slightly more than that, I do merge requests-list-headers-wrapper and requests-list-headers.
The header-group should be an immediate child of <table> (in existing code and still in your patch there is a wrapper in between <table> and <thead>[<thead> is the header-group]) and the table-cell should be immediate children of the header-group (that is already the case)
This is important for clarity and performance and you will have to rebase against this particular setup.

• removed position:absolute from <div.requests-list-contents>

I removed it too. To do that I also moved the "ref" in RequestListContent's render method, but ended up keeping the same name "contentEl". I'll try to better align with your naming of things.

• added table-layout: fixed to <div.requests-list-table> and removed position: relative

We should verify that table-layout: fixed is having any impact, and remove it if it doesn't.

• added AGAIN to <div.request-list-item> display: table-row; and removed position: relative (although I'm not sure here)

I do that in my patch, so you should should rebase on it.

• renamed <div.requests-list-wrapper> to <div.requests-list-scroll> because that's the one that takes care of the scrolling (we couldn't get rid of it because it is NOW used for scroll)

I'll include this renaming in my patch so that it is easier for you to rebase.
Note that I'm also renaming requests-list-contents to requests-list-row-group in order to ease the comprehension of the various elements.

• I have done my best to simplify the CSS although I am no expert :-) and you can certainly give more insight

Please, take a look at the latest patch in bug 1358414 so you can see the exact DOM structure there. It would really help if we could more or less go the same direction regarding structural changes that you do in this bug and that we do in the project for resize-columns. It could save some work. Thanks so much!

Sure, I'll tweak a few things to help you rebase on it, but you will have to still resolve a few conflicts.
I can help you resolving them if that's an issue for you.

I think it would be helpful for you to identify parts of your patch that can be landed independantly, before the main modification. The patch I submitted here is the perfect example. It doesn't depend on column resize, it doesn't regress the perf and help reducing the size of your patch. After rebasing, your patch should be smaller and more focus on resizing specifics. It should make it much easier to review it, have passing tests and figure out any performance issue.

Here is a new patch which should help aligning with bug 1358414's patch.
For example, I used "requests-list-scroll" className and "scrollEl" ref. And I also removed the onResize method.

But you will still have to adapt to changes made to RequestListHeader where I removed requests-list-headers-wrapper in order to merge it with requests-list-headers. Hopefully, you don't need such intermediate DOM Element for the resize...?
This change is important to keep a "standard" table structure and avoid any unexpected performance issue.

Again, if you have any issue with the rebase, I can help you.

Here is a try run for the latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=229449238&revision=b02da8ce6c54ee06c9f54db9b39383b1e8b79884

(In reply to Alexandre Poirot [:ochameau] from comment #11)

Alex is awesome!

I'm doing slightly more than that, I do merge requests-list-headers-wrapper and requests-list-headers.
The header-group should be an immediate child of <table> (in existing code and still in your patch there is a wrapper in between <table> and <thead>[<thead> is the header-group]) and the table-cell should be immediate children of the header-group (that is already the case)
This is important for clarity and performance and you will have to rebase against this particular setup.

I don't understand this comment.

1. Proper table structure is as follows:

<table>
    <thead>
        <tr>
            <td>Status</td>
            <td>Method</td>
        </tr>
    </thead>
    <tbody>
        <tr>
            <td>200</td>
            <td>GET</td>
        </tr>
    </tbody>
</table>

Lenka's patch is generating the following structure:

<div class="requests-list-table">                             <table>     display: table; table-layout: fixed;
    <div class="requests-list-headers-wrapper">               <thead>     display: table-header-group;position: sticky;
        <div class="requests-list-headers">                   <tr>        display: table-row; position: relative;
            <div class="requests-list-column">Status</div>    <td>        display: table-cell; overflow: hidden;
            <div class="requests-list-column">Method</div>    <td>        display: table-cell; overflow: hidden;
        </div>                                                </tr>
    </div>                                                    </thead>    
    <div class="requests-list-contents">                      <tbody>     display: table-row-group;
        <div class="request-list-item">                       <tr>        display: table-row;   // <<<<<< THIS CAUSES REGRESSION
            <div class="requests-list-column>200</div>        <td>        display: table-cell;overflow: hidden;
            <div class="requests-list-column">GET</div>       <td>        display: table-cell;overflow: hidden;
        </div>                                                </tr>
    </div>                                                    </tbody>  
</div>     

That is reasonable table structure and I don't see any extra elements.

Alex's patch is doing:

<div class="requests-list-table">                                                   <table>   display: table
    <div class="requests-list-headers">                                             <thead>   display: table-header-group; position:sticky
        <div class="requests-list-column"></div>                                    <td>      display: table-cell
        <div class="requests-list-column"></div>                                    <td>      display: table-cell
    </div>                                                                          </thead>
    <div class="requests-list-row-group">                                           <tbody>   display: table-row-group
        <div class="request-list-item">                                             <tr>      display: table-row
            <div class="requests-list-column">200</div>                             <td>      display: table-cell
            <div class="requests-list-column">GET</div>                             <td>      display: table-cell
        </div>                                                                      </tr>
    </div>                                                                          </tbody>
</div>                                                                              </table>

Note that <thead> should have <tr> as the immediate child - this is missing.

2. Another thing, the status bar is not aligned at the bottom if there is just one request. This can be solved e.g. by having height: 100% on .requests-list-scroll element (if it isn't perf issue)

3. Auto scroll at the bottom is broken

I'll be yet testing, but I don't see other any issue atm.

Thanks,
Honza

Depends on D20383

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #13)

Note that <thead> should have <tr> as the immediate child - this is missing.

I adressed that. So I should be even more similar now.

2. Another thing, the status bar is not aligned at the bottom if there is just one request. This can be solved e.g. by having height: 100% on .requests-list-scroll element (if it isn't perf issue)

Actually, that was caused by the removal of onResize, whereas setting fixed sizes in JS was still an important detail to prevent uncessary repaints.
I reverted that back and the size is now correct.

3. Auto scroll at the bottom is broken

Doing auto-scroll is now harder. I don't fully understand why. Now, the table changes its size even when we don't add new rows, but when we change some random properties on the rows.
I imagine that the sizes are less "fixed" with the new architecture, leading to changes when we update a label, an icon or something.
It means that we have to compute shouldScrollBottom on every react update. I do that in this patch, but I'm expecting that to have a negative impact on the performance.

It may be useful to play the the css auto-scroll feature to see if that works better...

Also, I piled up another changeset, optional, which would greatly help the comprehension and all the discussion around netmonitor DOM... This patch is converting div into real table elements. So that we use <table>, <tr>, <td>,...
I know this will add even more rebase conflicts but really, on the long run, it will be easier to communicate :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3689ae051ac1c5efa6041f9faa34f7396a624c32

(In reply to Alexandre Poirot [:ochameau] from comment #15)

Doing auto-scroll is now harder. I don't fully understand why. Now, the table changes its size even when we don't add new rows, but when we change some random properties on the rows.
I imagine that the sizes are less "fixed" with the new architecture, leading to changes when we update a label, an icon or something.
It means that we have to compute shouldScrollBottom on every react update. I do that in this patch, but I'm expecting that to have a negative impact on the performance.

You might try the following:

isScrolledToBottom() {
const { rowGroupEl, scrollEl } = this.refs;
const lastChildEl = rowGroupEl.lastElementChild;

if (!lastChildEl) {
return false;
}

const lastNodeHeight = lastChildEl.clientHeight;
return scrollEl.scrollTop + scrollEl.clientHeight >=
scrollEl.scrollHeight - lastNodeHeight / 2;
}

Talos doesn't indicate any perf regressions:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=ad68fcc809cd269a383bda0cc0720a2d25f9acb7&newProject=try&newRevision=04f1a077d50f1a9cd600aa36d25212762c033fe8&originalSignature=1759151&newSignature=1759151&framework=12

Honza

Thanks Honza for the scroll fix, I merged that in in the patch and tried to fix eslint and mochitest failure.
Let's see if try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62d3336e0abb3687b08a73a43bf9583a8be42548

Thanks, Alex, for your great work.

Let's see if try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62d3336e0abb3687b08a73a43bf9583a8be42548

Most of these failing mochitests are already fixed in this patch. They are failing because of the changed structure.
Lenka

Alex, patch fixing all test failures attached.
Try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae3406371f38fa77e6ec5bd33fcbb8be1cd59623

Honza

Both original patches look good to me, but before R+ I'd like to have feedback from Lenka too.

Honza

Sorry for not having posted a new try build, but the patches on phabricator were actually already green on try and do not need any other fixes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10a8db6ac6b7cf122a5571a7d945574bd5484311

At the end I addressed the test failure differently, I'm not introducing any new wait call.

It would be interesting to rebase Lenka's patch on top of this one and see if there is still a regression on DAMP.
Keep in mind that the onResize method can be still relevant to not regress the performance, so I would suggest keeping this code.

But in any case, we should proceed with this patch it can only simplify working on column resize patch.
By reducing the patch size, it will be easier to have a green try and understand from which particular modification the performance could possibly regress.

Thank you, Alex.
I looked at both patches and it looks great!

• I only noticed one difference with my logic - Alex is setting event listener for scrolling to scrollEl (which wraps the whole table) and I am setting it to contentEl (-> now requests-list-row-group) which just wraps the requests.
• also we have differently approached the browser_net_autoscroll.js test when we were fixing it. But if it works, than great! :-) I will import the patch and test it.

And do I understand correctly that at this point (with these 2 patches) we have no change in performance (improvements nor regressions)?

thanks again,
Lenka

Here is one last DAMP that confirms there is no impact on perf:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=198cd4a81bf2afa7cc79360f90da7bc91218b76d&newProject=try&newRevision=3dd27f24b76d73c1d9f6a7fe45e69a71cc0f6d40&originalSignature=1760367&newSignature=1759151&framework=12

And a green try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dd27f24b76d73c1d9f6a7fe45e69a71cc0f6d40

https://hg.mozilla.org/mozilla-central/rev/6b23faa481d9
https://hg.mozilla.org/mozilla-central/rev/91d3c9e78ce7