Currently our CSS Grid highlighter uses a canvas that covers the entire document to draw the grid's overlay. That is not a method that can be used, since depending by the GPU and by document's size, it's likely ending up on canvas size that is impossible to process for Firefox. There are several limitations to the size a <canvas> can have: 1. In SkiaGL (the library both Firefox and Chrome uses) the maximum size of a canvas is by default the GPU maximum texture size. That is made for HW acceleration. In my MBP for instance this value is just 4096, so it means the maximum canvas size would be 4096x4096. We have a fallback to SW if we exceeds this value (see bug 848490), but then the performance will drops since HW acceleration won't be used (see bug 1161818). 2. We have a limit of memory allocation for a canvas, that is currently 500.000.000 bytes (http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#401 it is also probable that we'll export this pref in the future, see Bug 1282656). That means, even if our GPU can handle a texture of 16384x16384, we can't have a canvas of this size, since exceeds the maximum allocation allowed, it's more than twice the size (16384 * 16384 = 268.435.456 pixels * 4 = 1.073.741.824 bytes). 3. Any size (in physical pixels) we decide to use, have to take in account the display dpi as well. For example, in a retina display with 2dppx, and GPU maximum texture size equals to 4096, we can have a canvas that is large 2048x2048 and no greater than that. To sum up, it's not practical create a canvas that cover an entire document, since it's likely it will be bigger than the values above. To solve this issue, we could make the canvas cover only the viewport, and draws the content in accordance with the page's scrolling (for a code example, see the patches of Bug 1144575). The problem here relies with the APZ. Since we rely on javascript's scrolling values, the rendering it will lag behind, and that was something we wanted to avoid (see Bug 1312103). We could then disabled the APZ in this specific scenario (as for the scrollable sub elements, see Bug 1316318) but that also is something we'd like to avoid if possible. The approach I'm currently investigating, and it seems promising, is the one suggested by :kats in bug 1316318 comment 10: having the canvas bigger than the viewport, so small scrolling won't cause any lag, but after the scroll exceeds a threshold, the canvas will be repositioned. It's something in between the two approaches above. Here the prototype: https://zer0.github.io/devtools/virtual-canvas.html [1] Exposing the display port to the web content (see Bug 1232491) could makes the algorithm more robust, the code cleaner, and maybe have a better performance, but I think it's already something we could try to implement in our current Grid Inspector. [1] The page will automatically use the maximum surface size from the GPU, within the limit of the memory allocation allowed. But it's possible specify a custom size, using the query string. It seems that 4096 gives better performance even on GPU that can handle greater texture, but I don't have enough machine to try to be statistically significant; so if you want to try: https://zer0.github.io/devtools/virtual-canvas.html?4096

So according to Matt at bug 1232491 comment 14 using a canvas larger than the texture size should be safe. I know nical also had some concerns about giant canvases and memory usage - Nical, do you have any objection to sizing the canvas to the displayport size? I think the best fix for this bug is to expose the displayport via an API (which we have been wanting to do for a while anyway) and then a displayport-sized canvas that moves around as you scroll to create a "virtual canvas" that's the size of the page.

Generally, trying to allocate the biggest canvas possible isn't a good approach. Running out of memory is the biggest source of instability in Firefox, so we should strive to avoid expensive allocations. It's probably not the worst thing in the world for a devtool feature because most users won't see it, but still it would be nice if using these tools wouldn't bring a low end computer to its knees. Canvas isn't particularly memory efficient due to the way it is shared between the content and compositor processes (plus some inefficiencies on our side) which amplifies the problem. Documents can be very large so allocating canvases the size of the document is indeed unrealistic. As a side note, I don't think that skia's GL backend should be in the equation. It's a good backend when you need to animate a scene that blits a lot of images and rectangles every frame (games type of things), but for mostly static content and things like strokes and dashes it should not make much of a difference (or it can slower in some cases). More importantly it's only on mac (and android but that doesn't matter in this case) so it won't affect the vast majority of our users. So if anything, it is better to think in terms of whether the canvas is backed by D2D or skia software (the backends we use on Windows). Having a large virtual canvas composed of one or several smaller canvases that scroll along with the page and are moved to fill the viewport seems like a good approach. It is somewhat similar to what currently happens under the hood in the graphics engine for regular content. In any case it is best to choose the canvas size in a way that minimizes the allocated area that isn't visible. I think that it is possible to create overlays with regular web content. Have you explored this possibility instead of using a canvas? The advantage is that the graphics engine will automatically do the job of only allocating pixels for the visible area and APZ won't be an issue anymore. If the type of things you are drawing in the overlay is mostly boxes, borders (dashed) lines and text, I don't think that canvas brings any technical advantage.

That's fair. Matteo, if it's possible to use regular web content (i.e. divs with borders) instead of a canvas that would be better. Regardless I think we need to expose the displayport API somewhere so I'll try and move that up in my list of things to do.

The problem with not using Canvas is the following: DevTools highlighters rely on the AnonymousContent API https://dxr.mozilla.org/mozilla-central/source/dom/base/AnonymousContent.h This API is a bit special in the sense that, although it lets chrome-JS inject HTML into the nsCanvasFrame of the document, it does not let you keep a reference to what you just injected. The only thing you can then do is change an attribute, a text content, etc. What that means, in turn, is that it's impossible to dynamically add/remove child nodes in what you injected. The only way would be to remove the injected node altogether and re-insert a new one, which makes it harder to write the code for, and possibly slower. The Grid highlighter is also special in that we can't know in advance how many lines, rectangles or labels we need to draw. So we can't create all the needed elements beforehand and then just change their text/position/visibility. This is the reason we chose canvas at first. Although I agree, it would make a lot more sense to use divs here! Maybe we can improve the AnonymousContent API to also allow add/removing nodes?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1) > So according to Matt at bug 1232491 comment 14 using a canvas larger than > the texture size should be safe. So, it's safe in the sense that it doesn't crash, but I have a couple of issues with that: 1. I didn't find a way to understand how much larger can be. For example, on a GPU that handles 4096 I can managing 5000, but If I try something greater it won't work. It's basically the same result reported in bug 1161818 comment 2. So first, if I don't have a way to obtain such number, I can't use it. 2. As also reported in bug 1161818 comment 2, the performance are decreasing too much; so it won't be suitable to use. As a side note, I noticed that also on a PC (windows) where the texture size is 16k, it handles much better canvas by 4096x4096 than 8192x8192; I was wondering if there is a specific reason, or it's just a regular decreasing performance due to the bigger size – I'm asking that because on 8192x8129 it should be still HW accelerated, if the texture size is 16k, right? (In reply to Nicolas Silva [:nical] from comment #2) > Generally, trying to allocate the biggest canvas possible isn't a good > approach. Running out of memory is the biggest source of instability in > Firefox, so we should strive to avoid expensive allocations. It's probably > not the worst thing in the world for a devtool feature because most users > won't see it, but still it would be nice if using these tools wouldn't bring > a low end computer to its knees. Canvas isn't particularly memory efficient > due to the way it is shared between the content and compositor processes > (plus some inefficiencies on our side) which amplifies the problem. > Documents can be very large so allocating canvases the size of the document > is indeed unrealistic. Agreed, I'm not planning to use gigantic canvas either, actually this bug is to avoid that as much as possible. > Having a large virtual canvas composed of one or several smaller canvases > that scroll along with the page and are moved to fill the viewport seems > like a good approach. It is somewhat similar to what currently happens under > the hood in the graphics engine for regular content. In any case it is best > to choose the canvas size in a way that minimizes the allocated area that > isn't visible. In that terms, is there any size that would be benefits more from what's happening under the hood? I would expects that power of 2 will be better and square canvas as well (2048x2048, 4096x4096…); but since there are layers of abstraction between the hw and the js/canvas code, maybe that's not actually true. > I think that it is possible to create overlays with regular web content. > Have you explored this possibility instead of using a canvas? The advantage > is that the graphics engine will automatically do the job of only allocating > pixels for the visible area and APZ won't be an issue anymore. If the type > of things you are drawing in the overlay is mostly boxes, borders (dashed) > lines and text, I don't think that canvas brings any technical advantage. Most of our overlays / highlighters are made by regular web content / SVG; but there are few cases where <canvas> is more suitable (rulers, grid inspector highlighter); especially when we have to write texts – otherwise we could end up with a very large DOM tree, and currently is not possible add / remove dynamically nodes in anonymous content; both issues makes impractical this approach in those scenarios.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #5) > 1. I didn't find a way to understand how much larger can be. For example, on > a GPU that handles 4096 I can managing 5000, but If I try something greater > it won't work. It's basically the same result reported in bug 1161818 > comment 2. So first, if I don't have a way to obtain such number, I can't > use it. The displayport API will expose the size that you should be using. You shouldn't need to discover this value yourself using other methods. > 2. As also reported in bug 1161818 comment 2, the performance are decreasing > too much; so it won't be suitable to use. I guess it's a question of how bad the perf is vs. how bad the perf will be with the other option we have (disabling APZ). I'm not sure which will have better perf characteristics. We would likely spend more effort optimizing the APZ-enabled scenario than the APZ-disabled scenario. That being said, on Windows at least our displayport should always be smaller than the max texture size, because tiling is disabled there and we explicitly clamp it [1]. On OS X we do have tiling enabled so we won't do the clamping, but given the majority of our userbase is on Windows I feel like we should hit the performance problem much less. (Unless the devtools userbase is skewed towards OS X?). [1] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/base/nsLayoutUtils.cpp#1088-1089

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; https://reviewboard.mozilla.org/r/124482/#review127180 I've tested this in a number of cases and didn't manage to break it :) It works really well, great job. I have a bunch of comments that are only related to jsdocs and comments and function names, but nothing related to the code change itself, it's easy enough to read and undersand. I do have a concern about testing though. I know we can't test the content of the canvas. But it seems like we should add a test for checking its position. Now that it's moving when you scroll, I think it's important that we check this still works correctly in the future. Do you want to do this in the same commit or in another one? I'll cancel R? for now. ::: commit-message-b43ad:1 (Diff revision 1) > +Bug 1345434 - implemented virtual canvas for grid highlighter; r=pbro nit: maybe rephrase the message to "Implementation of a virtual canvas technique for grid highlighter" And then add multiple lines below that to explain what we mean by virtual canvas here. ::: devtools/server/actors/highlighters/css-grid.js:62 (Diff revision 1) > -// It might become accessible as user preference, but at the moment we have to hard code > -// it (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1282656). > +// Then, we move around the element when and if needed, to give the perception we have a > +// bigger <canvas> (See bug 1345434). nit: rephrase: "Then, we move the element around when needed, to give the perception that it always covers the screen". ::: devtools/server/actors/highlighters/css-grid.js:65 (Diff revision 1) > -// One pixel on canvas is using 4 bytes (R, G, B and Alpha); we use this to calculate the > -// proper memory allocation below > +// This value is the safest value we could use as <canvas> size, since mostly any GPU can > +// handle it; and it's far from be close to the memory allocation limit per canvas nit: some rephrasing needed here too: "This canvas size value is the safest we can use because most GPUs can handle it. It's also far from the maximum canvas memory allocation limit" ::: devtools/server/actors/highlighters/css-grid.js:71 (Diff revision 1) > -// proper memory allocation below > -const BYTES_PER_PIXEL = 4; > -// The maximum allocable pixels the canvas can have > -const MAX_ALLOC_PIXELS = MAX_ALLOC_SIZE / BYTES_PER_PIXEL; > -// The maximum allocable pixels per side in a square canvas > -const MAX_ALLOC_PIXELS_PER_SIDE = Math.sqrt(MAX_ALLOC_PIXELS)|0; > +// handle it; and it's far from be close to the memory allocation limit per canvas > +// (4096x4096x4 is 67.108.864 bytes, where the limit is 500.000.000 bytes, see: > +// http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#401). > +// > +// Note: > +// Once bug 1232491 lands, we could try to refactoring this code to use the values from nit: "refactor" instead of "refactoring". ::: devtools/server/actors/highlighters/css-grid.js:72 (Diff revision 1) > -const BYTES_PER_PIXEL = 4; > -// The maximum allocable pixels the canvas can have > -const MAX_ALLOC_PIXELS = MAX_ALLOC_SIZE / BYTES_PER_PIXEL; > -// The maximum allocable pixels per side in a square canvas > -const MAX_ALLOC_PIXELS_PER_SIDE = Math.sqrt(MAX_ALLOC_PIXELS)|0; > +// (4096x4096x4 is 67.108.864 bytes, where the limit is 500.000.000 bytes, see: > +// http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#401). > +// > +// Note: > +// Once bug 1232491 lands, we could try to refactoring this code to use the values from > +// the displayport API instead. Do we need to reference a bug number here so people know what displayport is? ::: devtools/server/actors/highlighters/css-grid.js:660 (Diff revision 1) > - let ratio = parseFloat((this.win.devicePixelRatio || 1).toFixed(2)); > - > + * The <canvas>'s position needs to be updated if the page scroll too much, in order > + * to give the illusion we have a bigger <canvas>. nit: "The <canvas>'s position needs to be updated if the page scrolls too much, in order to give the illusion that it always covers the viewport" ::: devtools/server/actors/highlighters/css-grid.js:664 (Diff revision 1) > - clearCanvas(width, height) { > - let ratio = parseFloat((this.win.devicePixelRatio || 1).toFixed(2)); > - > - height *= ratio; > - width *= ratio; > + /** > + * The <canvas>'s position needs to be updated if the page scroll too much, in order > + * to give the illusion we have a bigger <canvas>. > + */ > + _scrollUpdate() { > + let hasPositionChanged = this.updateCanvasPosition(); You don't use this variable anywhere else than inside the `if` condition statement, so maybe we don't need to initialize the variable at all, and just call the function inside the condition. ::: devtools/server/actors/highlighters/css-grid.js:683 (Diff revision 1) > - "the size of the document inspected\n" + > - "See https://bugzilla.mozilla.org/show_bug.cgi?id=1343217 " + > + let bufferSizeX = (cssCanvasSize - viewportSize.width) >> 2; > + let bufferSizeY = (cssCanvasSize - viewportSize.height) >> 2; Please add a comment above these 2 lines that explain what the buffer is for and how it is calculated. ::: devtools/server/actors/highlighters/css-grid.js:723 (Diff revision 1) > } > > + return hasUpdated; > + }, > + > + clearCanvas(width, height) { clearCanvas isn't a self explanator name any longer, because not only does it clear it, but it also moves it now. In fact updateCanvasPosition doesn't update it, it only stores the position in a property. I think we should make that clear. Both in the jsdoc of updateCanvasPosition, also by renaming clearCanvas to clearAndMoveCanvas maybe, and by updating the line comment that is below ("Resize the canvas taking....") so it also mentions that we are moving it too.

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; https://reviewboard.mozilla.org/r/124482/#review127180 > Do we need to reference a bug number here so people know what displayport is? Is there already, at the beginning of the sence (the line above). > You don't use this variable anywhere else than inside the `if` condition statement, so maybe we don't need to initialize the variable at all, and just call the function inside the condition. I was in doubt myself about it, I added because it seems to me clearer in this way than: `if (this.updateCanvasPosition()) {…}` Having a variable named `hasPositionChanged` makes clear why we're in the `if` block at first sight (`if (hasPositionChanged) {…}`) without the needs to looking for `updateCanvasPosition` method. > clearCanvas isn't a self explanator name any longer, because not only does it clear it, but it also moves it now. > In fact updateCanvasPosition doesn't update it, it only stores the position in a property. I think we should make that clear. Both in the jsdoc of updateCanvasPosition, also by renaming clearCanvas to clearAndMoveCanvas maybe, and by updating the line comment that is below ("Resize the canvas taking....") so it also mentions that we are moving it too. My idea is tend to a similar approach in naming / code we have in react / redux nowadays, so the actions / methods just updates the state / properties, and the logic of how they should be update is there, where the "rendering" part is a dull thing that apply the state without much logic behind. I was thinking that maybe `clearCanvas` could be renamed `updateCanvasElement` and mention in the comment that this function is also doing the clearing too.

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; https://reviewboard.mozilla.org/r/124482/#review127180 I filed Bug 1352988 as follow up for the unit test. This is a patch that can't be covered just by automated tests, and it's important to land ASAP, so it's better avoid to delay it because the limited tests support we can provide.

Even though we display a message in the webconsole when the problem occurs, I think most people will miss it since the message appears in a different panel (webconsole) from the one being used at the moment (inspector). Just discussed with :zer0 on IRC, and it looks like we should be able to get this upflifted to 53. Let's make sure we don't forget about it! Marking the bug as affecting releases from 53 to 55.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10) > > Do we need to reference a bug number here so people know what displayport is? > > Is there already, at the beginning of the sence (the line above). Oh sorry, somehow missed that. > > You don't use this variable anywhere else than inside the `if` condition statement, so maybe we don't need to initialize the variable at all, and just call the function inside the condition. > > I was in doubt myself about it, I added because it seems to me clearer in > this way than: > `if (this.updateCanvasPosition()) {…}` > Having a variable named `hasPositionChanged` makes clear why we're in the > `if` block at first sight (`if (hasPositionChanged) {…}`) without the needs > to looking for `updateCanvasPosition` method. Ok for me. > > clearCanvas isn't a self explanator name any longer, because not only does it clear it, but it also moves it now. > > In fact updateCanvasPosition doesn't update it, it only stores the position in a property. I think we should make that clear. Both in the jsdoc of updateCanvasPosition, also by renaming clearCanvas to clearAndMoveCanvas maybe, and by updating the line comment that is below ("Resize the canvas taking....") so it also mentions that we are moving it too. > > My idea is tend to a similar approach in naming / code we have in react / > redux nowadays, so the actions / methods just updates the state / > properties, and the logic of how they should be update is there, where the > "rendering" part is a dull thing that apply the state without much logic > behind. > I was thinking that maybe `clearCanvas` could be renamed > `updateCanvasElement` and mention in the comment that this function is also > doing the clearing too. When the code actually uses react/redux, it's obvious, but here it's not the case. So having a method called updateCanvasPosition that doesn't actually update the position is sort of weird I think. In fact, you even have a comment that says: > Updates the <canvas> element's style in accordance with the current window's > devicePixelRatio, and the position calculated in `updateCanvasPosition` Having to write this comment, to me, means that the name isn't as straightforward as it could be. The comment says "updates the <canvas> element", so the method name updateCanvasElement is perfect, but then it says "the position calculated in updateCanvasPosition", so why is this other method not named calculateCanvasPosition. I don't want to block landing/uplifting on bikesheding, but I really like when functions have self-explanatory names, and I feel like right now, updateCanvasPosition is a bit misleading and too close to updateCanvasElement. (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11) > I filed Bug 1352988 as follow up for the unit test. This is a patch that > can't be covered just by automated tests, and it's important to land ASAP, > so it's better avoid to delay it because the limited tests support we can > provide. Ok, agreed. But please work on that test bug next so we don't forget.

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; https://reviewboard.mozilla.org/r/124482/#review128852

Pushed by mferretti@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/194a92719649 Implementation of a virtual canvas technique for grid highlighter; r=pbro

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; Approval Request Comment [Feature/Bug causing the regression]: Bug 1343217 [User impact if declined]: Grid Inspector won't be displayed properly in large document (some grids could be not highlighted at all) [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Yes [List of other uplifts needed for the feature/fix]: See bug 1343217 [Is the change risky?]: It shouldn't. [Why is the change risky/not risky?]: It impact only the grid highlighter, displayed from the grid inspector (that is a new feature). [String changes made/needed]: None. Notice that this is a request to uplift on 54. Since I'm not sure if Aurora is still here I asked for both Aurora and Beta uplift.

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

I tested this issue on iMac OS X 10.10, Macbook Pro OS X 10.12 with Retina display, Windows 10 x64 and Ubuntu 16.04 x64 with FF Nightly 55.0a1(2017-04-25) and I can confirm the fix. In order to verify this issue, I followed the steps from bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1343217 Thanks Matteo for your help!

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; Fix a large document handling issue in grid Inspector and was verified. Beta54+. Should be in 54 beta 4.

Needs a rebased patch for Beta uplift.

We should probably cancel the Beta approval here given that it's getting way too late for a rebased patch.

Comment on attachment 8852262 [details] Bug 1345434 - Implementation of a virtual canvas technique for grid highlighter; I agree with Ryan. This is very late beta and the patch is huge. It might have some risk with rebase patch. Beta54-. We can let this ride the train.