Bug 140602 - Remove the SVG instance tree
Summary: Remove the SVG instance tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
: 121050 (view as bug list)
Depends on: 140679 140826 140875 140984 141025 141108 141148 141374
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-18 17:58 PST by Philip Rogers
Modified: 2015-04-22 14:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (109.68 KB, patch)
2015-02-08 15:52 PST, Darin Adler
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2015-01-18 17:58:56 PST
Supporting the instance tree requires a lot of complexity and it can probably be removed without affecting any users.

The instance tree was ahead of it's time (many of the same issues are on public-webapps right now!) but it never caught on with users. Chromium's usecounters found no usage of instanceRoot and it was removed entirely in mid-2014[1]. I'm not aware of any web compatibility fallout from removing it (not a single bug, AFAIK). There is support in the SVGWG for removing the instance tree in SVG2[2]. The instance tree was never implemented in Gecko.

Removing the instance tree in blink took a few patches but most were mechanical. All the patches are listed on crbug.com/313438 and would be pretty easy to port to WebKit.

[1] https://groups.google.com/a/chromium.org/d/msg/blink-dev/mgYCJuzfU0Q/8jdhjY0pNgoJ
[2] https://lists.w3.org/Archives/Public/www-svg/2014Jan/0014.html
Comment 1 Darin Adler 2015-01-19 13:31:10 PST
Sounds good to me.
Comment 2 Sam Weinig 2015-01-19 17:34:50 PST
I'm fine with this as well.
Comment 3 Darin Adler 2015-01-20 08:21:26 PST
I’m going to take a crack at this.
Comment 4 Darin Adler 2015-01-20 08:40:31 PST
Investigating crbug.com/313438 further, it’s clear this is a pretty big project that Rob Buis did! But we can indeed take advantage of those patches and redo this for WebKit; it will be 10 or so patches to do it all. I am about to land a first patch.
Comment 5 Darin Adler 2015-02-08 15:52:30 PST
Created attachment 246251 [details]
Patch
Comment 6 Darin Adler 2015-02-08 17:13:40 PST
Committed r179810: <http://trac.webkit.org/changeset/179810>
Comment 7 Chris Dumez 2015-02-09 10:12:02 PST
This may have caused a perf regression on the HTML5 full-render test:
- https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D (Mavericks / 3.6% regression)
- https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22efl%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D (EFL / 2.3% regression)

I am not 100% sure it is caused by this change but the range on the EFL bot is small enough that I'd say it is very likely.
Comment 8 Chris Dumez 2015-02-09 10:18:03 PST
(In reply to comment #7)
> This may have caused a perf regression on the HTML5 full-render test:
> -
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-
> mavericks%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D (Mavericks /
> 3.6% regression)
> -
> https://perf.webkit.org/
> #mode=charts&chartList=%5B%5B%22efl%22%2C%22Parser%2Fhtml5-full-
> render%3ATime%22%5D%5D (EFL / 2.3% regression)
> 
> I am not 100% sure it is caused by this change but the range on the EFL bot
> is small enough that I'd say it is very likely.

Looking at the change though, I really don't see what could have caused a regression. I think Ryosuke is planning to confirm the regression locally (may be a flake on the bot or another commit).
Comment 9 Darin Adler 2015-02-09 14:05:48 PST
Are there any SVG <use> elements in the full-render test?
Comment 10 Said Abou-Hallawa 2015-04-22 14:43:28 PDT
*** Bug 121050 has been marked as a duplicate of this bug. ***