Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(406)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2232183002: Revert of SkLiteDL: turn vtable sideways (Closed)

Created:
4 years, 4 months ago by reed1
Modified:
4 years, 4 months ago
Reviewers:
mtklein, mtklein_C
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Revert of SkLiteDL: turn vtable sideways (patchset #7 id:120001 of https://codereview.chromium.org/2231103002/ ) Reason for revert: speculative revert to fix DEPS roll Original issue's description: > SkLiteDL: turn vtable sideways > > Instead of using virtuals for polymorphism, store each polymorphic operation > in its own array of function pointers. This removes an indirection when calling > the function, and lets us use nullptr as a signal to skip the call entirely. > > Unfortunately (as the old code is rather simpler) this makes an unambiguous speed improvement. > > Before: > curr/maxrss loops min median mean max stddev samples config bench > 21/26 MB 2 44.6µs 46.4µs 48.3µs 274µs 13% 51724 nonrendering desk_nytimes.skp > 23/26 MB 11 11.4µs 11.9µs 12.4µs 75.1µs 15% 36716 nonrendering keymobi_nytimes_com_.skp > > After: > 19/26 MB 4 40.4µs 41.6µs 42.5µs 197µs 10% 29379 nonrendering desk_nytimes.skp > 20/26 MB 14 10.5µs 10.7µs 11.2µs 59.5µs 13% 31971 nonrendering keymobi_nytimes_com_.skp > > Broader comparison: > keymobi_linkedin.skp 1.76us -> 1.77us 1x > keymobi_bing_com_search_q_sloth.skp 5.09us -> 5.05us 0.99x > keymobi_digg_com.skp 17.4us -> 17.3us 0.99x > keymobi_theverge_com.skp 3.37us -> 3.34us 0.99x > top25desk_mail_google_com_mail_.skp 30.8us -> 30.4us 0.99x > tabl_gmail.skp 3.44us -> 3.38us 0.98x > top25desk_wikipedia__1_tab_.skp 100us -> 97.7us 0.98x > keymobi_cnn_com_2012_10_03_poli.skp 52.9us -> 51.7us 0.98x > desk_chalkboard.skp 107us -> 104us 0.97x > desk_css3gradients.skp 17.8us -> 17.3us 0.97x > keymobi_androidpolice_com_2012_.skp 42.3us -> 41.1us 0.97x > desk_googlehome.skp 1.94us -> 1.88us 0.97x > keymobi_mlb_com_.skp 5.38us -> 5.18us 0.96x > top25desk_pinterest.skp 92.1us -> 88.5us 0.96x > keymobi_iphone_capitolvolkswage.skp 15.1us -> 14.5us 0.96x > keymobi_techcrunch_com.skp 9.45us -> 9.05us 0.96x > desk_espn.skp 31.3us -> 30us 0.96x > keymobi_slashdot_org_.skp 8.72us -> 8.34us 0.96x > desk_tiger8svg.skp 30.6us -> 29.2us 0.96x > keymobi_blogger.skp 4.09us -> 3.91us 0.95x > keymobi_baidu_com_s_wd_barack_o.skp 11.9us -> 11.3us 0.95x > keymobi_cuteoverload_com.skp 54.2us -> 51.6us 0.95x > keymobi_deviantart_com_.skp 17.2us -> 16.4us 0.95x > desk_mapsvg.skp 163ns -> 155ns 0.95x > keymobi_pinterest.skp 6.97us -> 6.62us 0.95x > top25desk_games_yahoo_com.skp 94.1us -> 89.3us 0.95x > top25desk_google_com_calendar_.skp 18us -> 17us 0.95x > keymobi_google_co_uk_search_hl_.skp 11.4us -> 10.8us 0.95x > tabl_pravda.skp 38.5us -> 36.4us 0.94x > desk_gmailthread.skp 19us -> 17.9us 0.94x > keymobi_reddit_com_r_programmin.skp 76.1us -> 71.7us 0.94x > top25desk_linkedin.skp 20us -> 18.8us 0.94x > tabl_gamedeksiam.skp 118us -> 112us 0.94x > keymobi_boingboing_net.skp 20.4us -> 19.1us 0.93x > top25desk_google_com__hl_en_q_b.skp 17.6us -> 16.4us 0.93x > keymobi_amazon_com_gp_aw_s_ref_.skp 12.5us -> 11.6us 0.93x > keymobi_sfgate_com_.skp 16.6us -> 15.5us 0.93x > keymobi_booking_com_searchresul.skp 16.2us -> 15.1us 0.93x > tabl_digg.skp 28.8us -> 26.8us 0.93x > tabl_mozilla.skp 80.4us -> 74.6us 0.93x > desk_wowwiki.skp 39.2us -> 36.4us 0.93x > top25desk_youtube_com.skp 42us -> 38.9us 0.93x > top25desk_facebook.skp 23.7us -> 22us 0.93x > top25desk_blogger.skp 38.2us -> 35.4us 0.93x > keymobi_online_wsj_com_home_pag.skp 12.8us -> 11.9us 0.93x > top25desk_wordpress.skp 28.9us -> 26.8us 0.93x > top25desk_answers_yahoo_com.skp 37.2us -> 34.4us 0.93x > keymobi_plus_google_com_app_bas.skp 9.56us -> 8.85us 0.93x > keymobi_wordpress.skp 16.1us -> 14.9us 0.92x > keymobi_mobile_news_sandbox_goo.skp 27.1us -> 24.9us 0.92x > top25desk_techcrunch_com.skp 31.1us -> 28.6us 0.92x > keymobi_worldjournal_com_.skp 50.7us -> 46.5us 0.92x > keymobi_theverge_com_2012_10_28.skp 26.2us -> 24us 0.92x > desk_jsfiddlebigcar.skp 1.73us -> 1.59us 0.92x > top25desk_weather_com.skp 31.3us -> 28.7us 0.92x > top25desk_google_com_search_q_c.skp 48.2us -> 44.1us 0.92x > top25desk_twitter.skp 27.8us -> 25.5us 0.92x > tabl_worldjournal.skp 29.3us -> 26.8us 0.91x > desk_nytimes.skp 46us -> 42us 0.91x > top25desk_docs___1_open_documen.skp 6.04us -> 5.51us 0.91x > keymobi_wikipedia__1_tab_.skp 59.7us -> 54.4us 0.91x > desk_unicodetable.skp 1.12ms -> 1.02ms 0.91x > top25desk_ebay_com.skp 17.8us -> 16.2us 0.91x > keymobi_ftw_usatoday_com_2014_0.skp 26.8us -> 24.3us 0.91x > top25desk_sports_yahoo_com_.skp 49.9us -> 45.3us 0.91x > keymobi_cnn_com.skp 9.94us -> 9.03us 0.91x > keymobi_m_youtube_com_watch_v_9.skp 13.4us -> 12.2us 0.91x > top25desk_amazon_com.skp 26.6us -> 24.1us 0.91x > keymobi_news_yahoo_com.skp 17.5us -> 15.8us 0.9x > keymobi_wowwiki_com_world_of_wa.skp 11.2us -> 10.2us 0.9x > top25desk_plus_google_com_11003.skp 93.5us -> 84.4us 0.9x > desk_carsvg.skp 53.5us -> 48.2us 0.9x > top25desk_news_yahoo_com.skp 44.7us -> 40.3us 0.9x > keymobi_wikipedia__1_tab____del.skp 59.4us -> 53.4us 0.9x > desk_googlespreadsheet.skp 66us -> 59.2us 0.9x > keymobi_answers_yahoo_com_quest.skp 30.2us -> 27us 0.89x > desk_ugamsolutions.skp 13us -> 11.6us 0.89x > keymobi_shop_mobileweb_ebay_com.skp 6.96us -> 6.21us 0.89x > keymobi_nytimes_com_.skp 12.1us -> 10.8us 0.89x > keymobi_gsp_ro.skp 5.54us -> 4.92us 0.89x > top25desk_booking_com.skp 54.9us -> 48.6us 0.89x > top25desk_espn.skp 37us -> 32.6us 0.88x > keymobi_facebook_com_barackobam.skp 23.3us -> 20.4us 0.88x > > BUG=skia: > GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231103002 > > Committed: https://skia.googlesource.com/skia/+/ac243914af957a806d842318a43dddaf5f941dc3 TBR=mtklein@google.com,mtklein@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia: Committed: https://skia.googlesource.com/skia/+/d230149ef889e920556949d2e50a1519b93aefea

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -171 lines) Patch
M src/core/SkLiteDL.h View 1 chunk +2 lines, -2 lines 0 comments Download
M src/core/SkLiteDL.cpp View 19 chunks +90 lines, -169 lines 0 comments Download

Messages

Total messages: 6 (2 generated)
reed1
Created Revert of SkLiteDL: turn vtable sideways
4 years, 4 months ago (2016-08-11 10:56:01 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2232183002/1
4 years, 4 months ago (2016-08-11 10:56:09 UTC) #3
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/d230149ef889e920556949d2e50a1519b93aefea
4 years, 4 months ago (2016-08-11 10:56:15 UTC) #5
reed1
4 years, 4 months ago (2016-08-11 10:57:57 UTC) #6
Message was sent while issue was closed. 
On 2016/08/11 10:56:15, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1) as
> https://skia.googlesource.com/skia/+/d230149ef889e920556949d2e50a1519b93aefea

FAILED: obj/skia/skia/SkLiteDL.o 
/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF obj/skia/skia/SkLiteDL.o.d -DV8_DEPRECATION_WARNINGS -DENABLE_MDNS=1
-DENABLE_NOTIFICATIONS -DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1
-DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1
-DENABLE_SPELLCHECK=1 -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_ASH=1 -DUSE_AURA=1
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DENABLE_WEBRTC=1
-DENABLE_EXTENSIONS=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_APP_LIST=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DIMAGE_LOADER_EXTENSION=1 -DENABLE_WAYLAND_SERVER=1 -DUSE_PROPRIETARY_CODECS
-DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
-DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=277962-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -DOS_CHROMEOS -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -DSK_IGNORE_DW_GRAY_FIX
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1
-DSK_GAMMA_EXPONENT=1.2 -DSK_GAMMA_CONTRAST=0.2
-DSK_DEFAULT_FONT_CACHE_LIMIT=20971520 -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_NOEXCEPT= -DU_STATIC_IMPLEMENTATION
-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen -I../../skia/config
-I../../skia/ext -I../../third_party/skia/include/c
-I../../third_party/skia/include/config -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/images
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu
-I../../third_party/skia/include/private
-I../../third_party/skia/include/client/android
-I../../third_party/skia/src/core -I../../third_party/skia/src/image
-I../../third_party/skia/src/opts -I../../third_party/skia/src/pdf
-I../../third_party/skia/src/ports -I../../third_party/skia/src/sfnt
-I../../third_party/skia/src/utils -I../../third_party/skia/src/lazy
-I../../third_party/zlib -I/usr/include/freetype2
-I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
-fno-strict-aliasing -funwind-tables -fPIC -pipe
-B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics
-fdebug-prefix-map=/b/c/b/linux_chromeos/src=. -pthread -m64 -march=x86-64 -O2
-fno-ident -fdata-sections -ffunction-sections -g1 -fvisibility=hidden -Xclang
-load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang
-add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs
-Xclang check-templates -Xclang -plugin-arg-find-bad-constructs -Xclang
follow-macro-expansion -Xclang -plugin-arg-find-bad-constructs -Xclang
check-implicit-copy-ctors -Xclang -plugin-arg-find-bad-constructs -Xclang
check-ipc -Werror -Wall -Wno-unused-variable -Wno-missing-field-initializers
-Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default
-Wno-deprecated-register -Wno-unneeded-internal-declaration
-Wno-inconsistent-missing-override -Wno-shift-negative-value
-Wno-undefined-var-template -Wno-nonportable-include-path
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti
-fno-exceptions -Wno-deprecated -Wno-reserved-user-defined-literal -c
../../third_party/skia/src/core/SkLiteDL.cpp -o obj/skia/skia/SkLiteDL.o
../../third_party/skia/src/core/SkLiteDL.cpp:741:43: error: no template named
'is_trivially_destructible' in namespace 'std'; did you mean
'has_trivial_destructor'?
static const void_fn dtor_fns[] = { TYPES(M) };
                                    ~~~~~~^~
../../third_party/skia/src/core/SkLiteDL.cpp:63:5: note: expanded from macro
'TYPES'
    M(Save) M(Restore) M(SaveLayer)                                            
\
    ^~~~~~~
../../third_party/skia/src/core/SkLiteDL.cpp:739:20: note: expanded from macro
'M'
#define M(T) !std::is_trivially_destructible<T>::value \
              ~~~~~^
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/type_traits:731:12:
note: 'has_trivial_destructor' declared here
    struct has_trivial_destructor
           ^

Powered by Google App Engine
This is Rietveld 408576698