PSA: Allowing //base types in core/ and modules/

[Kentaro Hara]

Hi

The arch team has been discussing what types should be allowed or disallowed in core/ and modules/ and reached the following decision [1].

Principle:

- We should share as much code between Chromium and Blink as possible and reduce the pain of using different types between the two code base. However, there are a couple of types that really need to be optimized for Blink's workload (WTF::Vector, WTF::HashTable, WTF::String etc). For those types we want to keep using Blink-specific types.

Guideline:

- core/ and modules/ can now directly use //base types and other primitive types in Chromium. You no longer need to define thin wrapper classes or aliases in platform/wtf/ to let core/ and modules/ use the //base types.

- For types that really need to be optimized for Blink's workload, you should use Blink-specific types defined in platform/wtf/. You should use WTF::Vector, WTF::HashTable, WTF::String, KURL etc instead of std::vector, std::map, std::string, GURL etc.

- We check the rule using a presubmit check. Only types that are whitelisted in the presubmit check are allowed in core/ and modules/. (This is not going to be a perfect check but it will work in 99% of the cases in practice.)

- platform/wtf/ provides helper functions to convert between Blink-specific types and Chromium types (e.g., std::string <=> WTF::String, GURL <=> KURL). Code that lives at the boundary between Blink and Chromium can use the helper functions.

I'm planning to add the guideline to a markdown file. If you have any question or concern, let me know! :D

[1] https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/eNLScUUWjCg/discussion

--

Kentaro Hara, Tokyo, Japan

[Kouhei Ueno]

SG

re: KURL and GURL, I'm not sure if it is really because KURL is currently "optimized for Blink's workload".

The only reason I can think of is that by having KURL you can selectively alter web exposing URL behavior (compared to GURL affecting the entire codebase).

If that's the only reason I'm leaning toward unifying KURL/GURL, but I am likely missing some additional context. Any thoughts welcome :)

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jwWSo94-F4MvQHhmjhFzW4Z9ym6oE2_ua%2BZn4EvTiKBWA%40mail.gmail.com.

--

kouhei

[Charles Harrison]

KURL vs GURL is a pretty major change in my opinion.

Some things off the top of my head:

- KURL is backed by WTF::String and GURL is backed by std::string so we'd be facing a lot of challenges with string conversions around usage of the GURL API.

- We copy KURLs around a lot since it is relatively cheap because we're backed by AtomicString in most cases. If we switched to GURL we will risk memory or CPU regressions. Some URLs like data URLs can be quite large (in the MBs).

- KURL has some specially optimized methods for e.g. accessing  

  - whether the URL is http or https

  - the atomic string protocol (http and https stored as a static string for fast comparisons)

I'm nervous a conversion could have significant CPU and memory regressions unless we made major changes to the consuming code. I'm also not confident it would be a code-health win due to introducing a std::string API to a huge portion of Blink.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAPVAxLUr-8eK1J%2BjhBhxSaJz_056u4HqfqqxW4YzkGWZ7SJ9_A%40mail.gmail.com.

[Kouhei Ueno]

Thanks for the insights! This is super useful. Would you add your bullet points as a document in KURL.h header file?

On Fri, Jul 14, 2017 at 12:54 PM, Charles Harrison <cshar...@chromium.org> wrote:

KURL vs GURL is a pretty major change in my opinion.

Some things off the top of my head:

- KURL is backed by WTF::String and GURL is backed by std::string so we'd be facing a lot of challenges with string conversions around usage of the GURL API.

- We copy KURLs around a lot since it is relatively cheap because we're backed by AtomicString in most cases. If we switched to GURL we will risk memory or CPU regressions. Some URLs like data URLs can be quite large (in the MBs).

- KURL has some specially optimized methods for e.g. accessing  

  - whether the URL is http or https

  - the atomic string protocol (http and https stored as a static string for fast comparisons)

I'm nervous a conversion could have significant CPU and memory regressions unless we made major changes to the consuming code. I'm also not confident it would be a code-health win due to introducing a std::string API to a huge portion of Blink.

Ideally, I think we can still merge the impls by:

- Splitting the string buffer backend / StringPiece ptrs to parsed components, or

- Use RefPtr<GURLwrapper> where we currently use KURL today (although this may still have memory impact by creating a separate std::string buffer along AtomicString one)

Given that, I totally agree that it is hard to justify the effort as of now.

--

kouhei

[Kinuko Yasuda]

Yes, KURL is primarily backed by WTF::String, which is more performance in many cases where we convert JS-String and KURLs back and forth.  There're also various perf implications as Charles pointed out.  (For some url conversions we use the same GURL utility code internally even in the current code though.)

There're ways to optimize or unify KURL/URL code path further, but in general I also don't think we should unify them at least now.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAPVAxLUPb5iCUKn2x6_Q79gGRCKQ4H5k-x2h5FK21HC5ZWtryQ%40mail.gmail.com.

[Charles Harrison]

Kouhei:

Yeah sure let me see about writing something up tomorrow. I have been procrastinating for the document-athon :)

Your ideas for making it work do make sense though. I am curious to see the future of std::string + StringPiece vs WTF::String + StringView, because I suspect lots of code will have to make tradeoffs like this.

[Raymond Toy]

On Thu, Jul 13, 2017 at 7:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

The arch team has been discussing what types should be allowed or disallowed in core/ and modules/ and reached the following decision [1].

Principle:

- We should share as much code between Chromium and Blink as possible and reduce the pain of using different types between the two code base. However, there are a couple of types that really need to be optimized for Blink's workload (WTF::Vector, WTF::HashTable, WTF::String etc). For those types we want to keep using Blink-specific types.

Guideline:

- core/ and modules/ can now directly use //base types and other primitive types in Chromium. You no longer need to define thin wrapper classes or aliases in platform/wtf/ to let core/ and modules/ use the //base types.

- For types that really need to be optimized for Blink's workload, you should use Blink-specific types defined in platform/wtf/. You should use WTF::Vector, WTF::HashTable, WTF::String, KURL etc instead of std::vector, std::map, std::string, GURL etc.

Is there any documentation explaining how WTF:Vector is better optimized for blink than std::vector? Similarly for HashTable and so on.

 

- We check the rule using a presubmit check. Only types that are whitelisted in the presubmit check are allowed in core/ and modules/. (This is not going to be a perfect check but it will work in 99% of the cases in practice.)

- platform/wtf/ provides helper functions to convert between Blink-specific types and Chromium types (e.g., std::string <=> WTF::String, GURL <=> KURL). Code that lives at the boundary between Blink and Chromium can use the helper functions.

I'm planning to add the guideline to a markdown file. If you have any question or concern, let me know! :D

[1] https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/eNLScUUWjCg/discussion

--

Kentaro Hara, Tokyo, Japan

--

[Kentaro Hara]

+yutak

On Sat, Jul 15, 2017 at 12:25 AM, Raymond Toy <rt...@google.com> wrote:

On Thu, Jul 13, 2017 at 7:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

The arch team has been discussing what types should be allowed or disallowed in core/ and modules/ and reached the following decision [1].

Principle:

- We should share as much code between Chromium and Blink as possible and reduce the pain of using different types between the two code base. However, there are a couple of types that really need to be optimized for Blink's workload (WTF::Vector, WTF::HashTable, WTF::String etc). For those types we want to keep using Blink-specific types.

Guideline:

- core/ and modules/ can now directly use //base types and other primitive types in Chromium. You no longer need to define thin wrapper classes or aliases in platform/wtf/ to let core/ and modules/ use the //base types.

- For types that really need to be optimized for Blink's workload, you should use Blink-specific types defined in platform/wtf/. You should use WTF::Vector, WTF::HashTable, WTF::String, KURL etc instead of std::vector, std::map, std::string, GURL etc.

Is there any documentation explaining how WTF:Vector is better optimized for blink than std::vector? Similarly for HashTable and so on.

Regarding WTF::Vector, the inline capacity is important for many performance-sensitive code paths. std::vector doesn't support the inline capacity.

Regarding WTF::HashTable, yutak@ would know better.

Another fundamental reason is that WTF::Vector and WTF::HashTable are tightly coupled with Oilpan. (It would be hard to implement HeapVector and HeapHashTable on top of std::vector and std::hash_map.)

 

 

- We check the rule using a presubmit check. Only types that are whitelisted in the presubmit check are allowed in core/ and modules/. (This is not going to be a perfect check but it will work in 99% of the cases in practice.)

- platform/wtf/ provides helper functions to convert between Blink-specific types and Chromium types (e.g., std::string <=> WTF::String, GURL <=> KURL). Code that lives at the boundary between Blink and Chromium can use the helper functions.

I'm planning to add the guideline to a markdown file. If you have any question or concern, let me know! :D

[1] https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/eNLScUUWjCg/discussion

--

Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jwWSo94-F4MvQHhmjhFzW4Z9ym6oE2_ua%2BZn4EvTiKBWA%40mail.gmail.com.

[Chris Harrelson]

This update sounds great. It will reduce friction and duplicated code between Blink and Chromium.

I have a request, however: when expanding the list of allowed types in a non-trivial way, I would like it to be required to have a PSA blink-dev email, plan for migration, and buy-in from leads in the affected code areas. This will avoid any inadvertent performance issues, and also avoid situations where half of the code is converted to a new type and the other half is not.

One set of examples I have in mind is graphics types like float rects, points and matrices. We should aim to unify those types between Blink and Chromium, but it will be a lot of work and should not be pursued until we have a well-thought-out and staffed plan to do so.

Also, is the mentioned presubmit already in place? I'd like to take a look and make sure examples like I mentioned above are excluded for the moment, with appropriate comment.

Thanks for driving this forward!

Chris

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jwJNOeGmtMZs5gmPza%3DWvRjfeb6eVcYKOAXz2hzNtGvAA%40mail.gmail.com.

[Kentaro Hara]

On Sat, Jul 15, 2017 at 2:25 AM, Chris Harrelson <chri...@chromium.org> wrote:

This update sounds great. It will reduce friction and duplicated code between Blink and Chromium.

I have a request, however: when expanding the list of allowed types in a non-trivial way, I would like it to be required to have a PSA blink-dev email, plan for migration, and buy-in from leads in the affected code areas. This will avoid any inadvertent performance issues, and also avoid situations where half of the code is converted to a new type and the other half is not.

+1 to sending a PSA when introducing non-trivial types to Blink.

I'm also planning to add an OWNER to review the whitelist in the presubmit script. I think that the OWNER review would be enough when introducing trivial types.

One set of examples I have in mind is graphics types like float rects, points and matrices. We should aim to unify those types between Blink and Chromium, but it will be a lot of work and should not be pursued until we have a well-thought-out and staffed plan to do so.

Also, is the mentioned presubmit already in place? I'd like to take a look and make sure examples like I mentioned above are excluded for the moment, with appropriate comment.

Not yet. The current presubmit script simply forbids "base::" in core/ and modules/.

[Brett Wilson]

On Thu, Jul 13, 2017 at 7:14 PM, 'Kouhei Ueno' via blink-dev <blin...@chromium.org> wrote:

SG

re: KURL and GURL, I'm not sure if it is really because KURL is currently "optimized for Blink's workload".

The only reason I can think of is that by having KURL you can selectively alter web exposing URL behavior (compared to GURL affecting the entire codebase).

If that's the only reason I'm leaning toward unifying KURL/GURL, but I am likely missing some additional context. Any thoughts welcome :)

I think we will want a different URL type for Blink as long as the string representations are different. There are a lot of conversions back and forth.

Brett

[Yuta Kitamura]

On Sat, Jul 15, 2017 at 2:15 AM, Kentaro Hara <har...@chromium.org> wrote:

+yutak

On Sat, Jul 15, 2017 at 12:25 AM, Raymond Toy <rt...@google.com> wrote:

On Thu, Jul 13, 2017 at 7:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

The arch team has been discussing what types should be allowed or disallowed in core/ and modules/ and reached the following decision [1].

Principle:

- We should share as much code between Chromium and Blink as possible and reduce the pain of using different types between the two code base. However, there are a couple of types that really need to be optimized for Blink's workload (WTF::Vector, WTF::HashTable, WTF::String etc). For those types we want to keep using Blink-specific types.

Guideline:

- core/ and modules/ can now directly use //base types and other primitive types in Chromium. You no longer need to define thin wrapper classes or aliases in platform/wtf/ to let core/ and modules/ use the //base types.

- For types that really need to be optimized for Blink's workload, you should use Blink-specific types defined in platform/wtf/. You should use WTF::Vector, WTF::HashTable, WTF::String, KURL etc instead of std::vector, std::map, std::string, GURL etc.

Is there any documentation explaining how WTF:Vector is better optimized for blink than std::vector? Similarly for HashTable and so on.

Regarding WTF::Vector, the inline capacity is important for many performance-sensitive code paths. std::vector doesn't support the inline capacity.

Regarding WTF::HashTable, yutak@ would know better.

WTF::HashTable uses a method called "open addressing" which only requires a single contiguous buffer to store the elements. On the other hand, std::unordered_set/map uses "chaining", where each element is stored in a dynamically allocated storage (like a linked list). The latter requires many dynamic allocations, and the latter is also bad for CPU cache. This is bad, just as std::map or std::list is bad.