Bug 30032 - [Gtk] Creation of a WebkitWebView widget is very slow
Summary: [Gtk] Creation of a WebkitWebView widget is very slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 420+
Hardware: PC Linux
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-02 15:49 PDT by Philip Chimento
Modified: 2010-02-24 22:29 PST (History)
8 users (show)

See Also:


Attachments
Timing test, WebkitWebView vs. GtkTextView (1.40 KB, text/x-csrc)
2009-10-02 15:49 PDT, Philip Chimento
no flags Details
Profiler output of timing test from sysprof (1001.07 KB, text/xml)
2009-10-05 14:14 PDT, Philip Chimento
no flags Details
Make EnchantBroker a singleton (11.01 KB, patch)
2009-11-21 09:51 PST, Krzysztof Kotlenga
no flags Details | Formatted Diff | Diff
Make EnchantBroker a singleton (updated) (10.55 KB, patch)
2010-01-20 05:19 PST, Krzysztof Kotlenga
eric: review-
Details | Formatted Diff | Diff
Updated patch (w/ ChangeLog & style issues fixed) (13.24 KB, patch)
2010-02-20 11:32 PST, Martin Robinson
gns: review+
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Chimento 2009-10-02 15:49:23 PDT
Created attachment 40554 [details]
Timing test, WebkitWebView vs. GtkTextView

I need to put a fair number of WebkitWebViews into one GTK application, and it takes over a second for my window to be built. (I am using Webkit-GTK 1.1.10.) The constructor webkit_web_view_new() is very slow.

Please see attached program for a timing test. I have compared creating twenty WebkitWebViews to creating twenty GtkTextViews, which is also a fairly complicated widget.

Compile with:
gcc -o testbug testbug.c `pkg-config --cflags --libs gtk+-2.0 webkit-1.0`

Typical output:
webkit_web_view_new() time: 0.036524
webkit_web_view_new() time: 0.041833
webkit_web_view_new() time: 0.029860
webkit_web_view_new() time: 0.028802
webkit_web_view_new() time: 0.028518
webkit_web_view_new() time: 0.028611
webkit_web_view_new() time: 0.028586
webkit_web_view_new() time: 0.028596
webkit_web_view_new() time: 0.029057
webkit_web_view_new() time: 0.028576
webkit_web_view_new() time: 0.028491
webkit_web_view_new() time: 0.028798
webkit_web_view_new() time: 0.028640
webkit_web_view_new() time: 0.028650
webkit_web_view_new() time: 0.031685
webkit_web_view_new() time: 0.028799
webkit_web_view_new() time: 0.028759
webkit_web_view_new() time: 0.028816
webkit_web_view_new() time: 0.029005
webkit_web_view_new() time: 0.029732
gtk_text_view_new() time: 0.000489
gtk_text_view_new() time: 0.000018
gtk_text_view_new() time: 0.000023
gtk_text_view_new() time: 0.000017
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000020
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000019
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000015
gtk_text_view_new() time: 0.000020
gtk_text_view_new() time: 0.000018
gtk_text_view_new() time: 0.000016
gtk_text_view_new() time: 0.000020
gtk_text_view_new() time: 0.000019
gtk_text_view_new() time: 0.000017
gtk_text_view_new() time: 0.000025
gtk_text_view_new() time: 0.000016
Total time: 0.714716
Comment 1 Xan Lopez 2009-10-05 05:32:42 PDT
Well, first of all GtkTextView is nowhere near as complex as WebKitWebView, since a webview comes with many hundreds of thousands of C++ lines behind.

That being said, just profile the creation of the widget with sysprof or similar and we'll see what's taking the time.
Comment 2 Philip Chimento 2009-10-05 14:14:50 PDT
Created attachment 40660 [details]
Profiler output of timing test from sysprof
Comment 3 Philip Chimento 2009-10-05 14:16:14 PDT
Thanks for the tip, I didn't know about sysprof. It is nifty. I hope I understood how to use it and interpret the output correctly. The output is attached, look for the program [testbug]. It seems that the bottleneck is enchant_broker_request_dict(), called in webkit_web_settings_set_property() in webkitwebsettings.cpp.

The default value for the "spell-checking-languages" property is NULL, meaning load the dictionary for GTK's default language, so at least one dictionary gets loaded whenever you create a WebView. Would it work to avoid loading the dictionary if the "enable-spell-checking" property is FALSE (as it is by default)?
Comment 4 Krzysztof Kotlenga 2009-11-21 09:51:35 PST
Created attachment 43653 [details]
Make EnchantBroker a singleton

According to http://sourceforge.net/tracker/?func=detail&aid=2642198&group_id=7896&atid=107896 EnchantBroker should be a singleton. This patch:
- solves the issue, obviously
- simplifies things (removal of SpellLanguage struct)
- renames some variables to better reflect what they are
- gives a more meaningful name to get_spell_languages method (get_enchant_dicts)
- adds a missing g_free()
- removes "spell-checking-languages-list" from the _copy() thing, because it's not even a property (?)
- applies to trunk too
- is written by an annoyed user, so please bear with me :)
Comment 5 Krzysztof Kotlenga 2010-01-20 05:19:20 PST
Created attachment 47009 [details]
Make EnchantBroker a singleton (updated)

This is an updated version of the patch above, so it applies without fuzz against WebKitGTK 1.1.19. No other changes were made.

How to see a really big slowdown caused by this bug:
1. get a copy of Polish myspell dictionary from http://www.sjp.pl/slownik/en/
2. extract pl_PL.{aff,dic} to ~/.config/enchant/myspell/
3. restart your favorite browser
4. cry
Comment 6 Alexander Butenko 2010-01-20 11:26:26 PST
I agree  Krzysztof, enchant does a crazy job during startup. You can feel with with multiply dictionaties installed. 

So me++ for the patch :) 

The problem is does patch really needs to break existing api? I think it can be done without api changes.
Comment 7 Alexander Butenko 2010-02-04 19:59:37 PST
ping? 

Patch still applies fine to current 1.1.20. Can it be reviewed?
Comment 8 Priit Laes (IRC: plaes) 2010-02-04 23:54:57 PST
(In reply to comment #7)
> ping? 
> 
> Patch still applies fine to current 1.1.20. Can it be reviewed?

You need to set the Review flag to "?" in patch details so it pops up on list of reviewable patches (unless you harass some developer on IRC or mailinglist to do it..) :S
Comment 9 Eric Seidel (no email) 2010-02-05 15:02:09 PST
Comment on attachment 47009 [details]
Make EnchantBroker a singleton (updated)

Every patch requires a ChangeLog.  This patch would not apply to our repository as is anyway due ot the "webkit-1.1.9" path prefixes.

See http://webkit.org/coding/contributing.html
Comment 10 Martin Robinson 2010-02-20 11:32:57 PST
Created attachment 49131 [details]
Updated patch (w/ ChangeLog & style issues fixed)

I think this is a very good change, so I've taken the liberty of cleaning the patch up a bit and attaching a ChangeLog.
Comment 11 WebKit Review Bot 2010-02-20 11:36:43 PST
Attachment 49131 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/290665
Comment 12 WebKit Review Bot 2010-02-20 11:37:54 PST
Attachment 49131 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebsettings.cpp:833:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Gustavo Noronha (kov) 2010-02-23 14:22:30 PST
Comment on attachment 49131 [details]
Updated patch (w/ ChangeLog & style issues fixed)

1276  GSList* webkit_web_settings_get_spell_languages(WebKitWebView *web_view)
 1272 GSList* webkit_web_settings_get_enchant_dicts(WebKitWebView* webView)
12771273 {
12781274     g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(web_view), 0);
12791275 
12801276     WebKitWebSettings* settings = webkit_web_view_get_settings(web_view);

Looks like you need to adjust the variable name in the two calls above =). r+ with that change, and removing the else clause, as pointed by the style bot, if you feel like committing it, Martin!
Comment 14 Martin Robinson 2010-02-24 22:29:12 PST
Committed r55222: <http://trac.webkit.org/changeset/55222>