svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.07k stars 1.08k forks source link

Improve Performance for .move() / .dmove() #1240

Closed Shiuyin closed 1 year ago

Shiuyin commented 2 years ago

This is somewhat related to https://github.com/svgdotjs/svg.js/issues/1214.

We have a huge chart being drawn with SVG.js that has a "sticky" header section consisting sometimes of up to hundreds or thousands of rect and text elements (The header itself is n simple group objects, containing the rects and texts). To implement the sticky behaviour we listen to onScroll listeners and then move the header sections to the new position. It looks a bit like this:

moveHeaderTo(y: number): void {
        let headerIndex = 0
        for (const header of this.svg.find('.' + CSSConstants.GANTT_HEADER)) {
            header.y(headerIndex * this.DEFAULT_ROW_HEIGHT + y)
            headerIndex += 1
        }
    }

When we implemented this we quickly realized that performance was bad. Scrolling in a tiny amount of randomly generated 70 rects and texts, the Chrome performance tools show that > 50% of time is used by SVG.js in the .y() call. The call chain ends always at .dmove() which tries to get the BBox and then position the elements according to the BBox and some matrix math.

At first I thought the text elements are (again) the culprit and removed them temporarily => Same problem.

In the end I implemented the following function, which is called by the function above instead of the header.y():

moveHeader(header: Element, y: number): void {
        for (const child of header.children()) {
            if (child.hasClass(CSSConstants.GANTT_HEADER_CELL + '')) {
                const cell = child as Rect
                cell.attr('y', y)
            } else if (child.hasClass(CSSConstants.GANTT_HEADER_CELL_LABEL + '')) {
                const label = child as Text
                const labelY = y + this.DEFAULT_ROW_HEIGHT / 2
                label.attr('y', labelY)
            }
        }
    }

This will bring the scripting time needed by SVG.js down to way below 5% and works with even thousands of randomly generated test objects.

I am not (yet) enough knowledgeable about SVG to say if something like this logic could be implemented as the default behaviour for .move() or .dmove(). At the end of the day, we just need to shift those header groups by a certain amount up or down. If the element's y attribute already has the correct coordinates prior to scrolling, what reason is there that a .dmove() call needs to access all BBox'es again instead of just incrementing or decrementing the y values in the attributes from before scrolling?

Are there other ways I have missed that might help this particular case?

I have also tried:

I think that in general trying to position via BBox() should be a last resort. Links I found the last hours trying to fix this problem like https://support.google.com/chrome/thread/118284571/any-one-suffers-from-the-newest-ver-92-rendering-some-heavy-svg-jobs?hl=en show that BBox() really is no good option (currently?) to use at 50+ elements. Any way to implement .move() and it's related cousins to not rely on BBox() would greatly help all users.

Fuzzyma commented 2 years ago

If you dmove a group, all its contents are dmoved. That can indeed result in bbox e.g. if the group itself has groups. However, for your use case you should just rely on transforms instead. They will just move the whole group with no magic and it might even render faster by the browser:

group.translate(0, -10) // move by y -10
Shiuyin commented 2 years ago

Thank you for your answer.

I would very much like to simply use .translate(), but currently MS Edge is bugged and renders the content of foreignObjects above those headers, even though the headers exist earlier in DOM than the foreignObject. This is related to .translate() as simply using .y() had less (but still some) of that problem.

Therefore, i currently cannot use translate...

Are there any other options?

Fuzzyma commented 2 years ago

No, not really. If Edge has the problem, chrome should have it, too. They both use the same rendering engine. Did you report the bug to the vendors already? Also you can try to use z-index on the content of the foreignObject maybe -1 works.

However, if your fix works for you, just go with that. You have more info about the dimensions of your elements avilable and therefore you can optimize for that.

chanon commented 1 year ago

I am currently running into this problem too (move() method being very slow). Will probably have to try setting x/y attributes directly too, but it would be nice if move() automatically chose the most performant method.

EDIT: I figured out that actually just setting x/y attributes together with transform="translate(x, y)" and transform-origin="x y" seems to be a simple performant way for placing items when you know what you want and all the values involved.

ebloemsma commented 1 year ago

We have the same issue that moving a group with ~ 3000 objects (lines) with the .y(..) function on the group, is very slow. Interestingly this occurred after switching from svg.js v2.7 to v3.x. Investigating with the chrome profiler showed that with v3 the browser spends much more time layouting than with v2

svg3: svg3_overview svg3_stack

SVG 2: svg2_overview svg2_stack image

With svg 2 the operation on the group takes 0.5ms layouting happens detached after the y(..) call has finished With svg 3 the operation takes 1300 ms and the layouting is "integrated" int the y(..) stacktrace

profiles.zip

Fuzzyma commented 1 year ago

The general advice for big groups is: Use transforms over move/size svg.js v2 mapped move to translate for groups. But this was inconsistent and lead to unexpected bounding boxes. The current solution avoids these inconsistencies and prevents users that are not experts in svg to run into bugs. However, you can still explicitely use transforms

Closing this since this is a wontfix