Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
256563002:
Populate the name-to-pseudo map lazily (Closed)
|
Created:
6 years, 8 months ago by fs Modified:
6 years, 7 months ago CC:
blink-reviews, ed+blinkwatch_opera.com, ojan, dglazkov+blink, apavlov+blink_chromium.org, darktears, rune+blink, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionPopulate the name-to-pseudo map lazily
Sort pseudoTypeMap[] to allow using binary search to look up the name.
BUG=365613
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173317
Patch Set 1 #
Total comments: 5
Patch Set 2 : Drop the HashMap; Binary search only. #Patch Set 3 : Drop unused include. #Messages
Total messages: 13 (0 generated)
The list is about 100 items long. It's not impossible that doing a binary search through it is fast enough and that the hash table is just unnecessary. That would result in even shorter and simpler code. And very slightly less memory used. But this looks like a good improvement to me as is as well.
https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp File Source/core/css/CSSSelector.cpp (right): https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... Source/core/css/CSSSelector.cpp:374: CString cstrName = name.utf8(); This could as well be inlined
https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp File Source/core/css/CSSSelector.cpp (right): https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... Source/core/css/CSSSelector.cpp:374: CString cstrName = name.utf8(); On 2014/04/24 15:19:18, apavlov wrote: > This could as well be inlined Assuming you by that mean: ... key = { name.utf8().data(), ... }; then, no, I don't think so, since the lifetime of the CString would then not last to the next line. Unfortunately.
https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp File Source/core/css/CSSSelector.cpp (right): https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... Source/core/css/CSSSelector.cpp:374: CString cstrName = name.utf8(); On 2014/04/24 16:01:53, fs wrote: > On 2014/04/24 15:19:18, apavlov wrote: > > This could as well be inlined > > Assuming you by that mean: > > ... key = { name.utf8().data(), ... }; > > then, no, I don't think so, since the lifetime of the CString would then not > last to the next line. Unfortunately. Ahh, correct. There's a newly constructed object in flight there...
Since esprehn is where the suggestion originated, I guess he should have a say now when the cat is out of the hat...
lgtm, I might binary search all the time like RenderQuote does, but this is fine too. https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp File Source/core/css/CSSSelector.cpp (right): https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... Source/core/css/CSSSelector.cpp:369: NameToPseudoTypeMap::iterator slot = nameToPseudoTypeMap.find(name); You might even be able to always binary search and get rid of the hash table entirely.
https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp File Source/core/css/CSSSelector.cpp (right): https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... Source/core/css/CSSSelector.cpp:369: NameToPseudoTypeMap::iterator slot = nameToPseudoTypeMap.find(name); On 2014/04/25 09:08:03, esprehn wrote: > You might even be able to always binary search and get rid of the hash table > entirely. Yepp, that's doable, but I figured it'd be too gnarly... Anyway, uploaded now, and probably warrants a new look.
On 2014/04/25 11:23:32, fs wrote: > https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp > File Source/core/css/CSSSelector.cpp (right): > > https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... > Source/core/css/CSSSelector.cpp:369: NameToPseudoTypeMap::iterator slot = > nameToPseudoTypeMap.find(name); > On 2014/04/25 09:08:03, esprehn wrote: > > You might even be able to always binary search and get rid of the hash table > > entirely. > > Yepp, that's doable, but I figured it'd be too gnarly... Anyway, uploaded now, > and probably warrants a new look. esprehn: Still L-G-T-M?
On 2014/04/29 07:17:11, fs wrote: > On 2014/04/25 11:23:32, fs wrote: > > > https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.cpp > > File Source/core/css/CSSSelector.cpp (right): > > > > > https://codereview.chromium.org/256563002/diff/1/Source/core/css/CSSSelector.... > > Source/core/css/CSSSelector.cpp:369: NameToPseudoTypeMap::iterator slot = > > nameToPseudoTypeMap.find(name); > > On 2014/04/25 09:08:03, esprehn wrote: > > > You might even be able to always binary search and get rid of the hash table > > > entirely. > > > > Yepp, that's doable, but I figured it'd be too gnarly... Anyway, uploaded now, > > and probably warrants a new look. > > esprehn: Still L-G-T-M? Ping!? --^
Sorry, still lgtm
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/256563002/40001
Message was sent while issue was closed.
Change committed as 173317 |
