Issue 11463 in v8: ExternalString's data cache

10 views
Skip to first unread message

sola… via monorail

unread,
Feb 17, 2021, 10:42:59 AM2/17/21
to v8-re...@googlegroups.com
Status: Assigned
Owner: sol...@chromium.org
CC: les...@chromium.org, u...@chromium.org
Components: Compiler Runtime
Priority: 2
Type: FeatureRequest

New issue 11463 by sol...@chromium.org: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463

Tracking bug for ExternalString's data caching.

Design doc: https://docs.google.com/document/d/101eAQqFpBPWFGNJicxtdlwYShJkTOUsEuxkVVeu5Hrk/edit?ts=60187499&pli=1#

Possible improvements:
- Teach TurboFan/CSA to use the cached value to avoid bailing out to the runtime.
- Introduce new types of Resources to avoid memory regressions for strings which do not use the cached_data_ (Maybe not needed, see next bullet point).
- Get rid of the on-V8-heap cache pointer for all external strings (except the uncached ones with uncacheable resources, but that should be rare).

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

sola… via monorail

unread,
Feb 17, 2021, 11:58:10 AM2/17/21
to v8-re...@googlegroups.com

Comment #1 on issue 11463 by sol...@chromium.org: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c1


Tracking bug for ExternalString's data caching.

Design doc: https://docs.google.com/document/d/101eAQqFpBPWFGNJicxtdlwYShJkTOUsEuxkVVeu5Hrk/edit?ts=60187499&pli=1#

Possible improvements:
- Teach TurboFan/CSA to use the cached value to avoid bailing out to the runtime.
- Introduce new types of Resources to avoid memory regressions for strings which do not use the cached_data_ (Maybe not needed, see next bullet point).
- Get rid of the on-V8-heap cache pointer for all external strings.

bugdroid via monorail

unread,
Feb 19, 2021, 7:21:49 AM2/19/21
to v8-re...@googlegroups.com

Comment #2 on issue 11463 by bugdroid: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c2

The following revision refers to this bug:
https://chromium.googlesource.com/v8/v8/+/ed225df70cc62b03942850ddd5493c365aff3157

commit ed225df70cc62b03942850ddd5493c365aff3157
Author: Santiago Aboy Solanes <sol...@chromium.org>
Date: Fri Feb 19 12:17:04 2021

[objects] Cache the ExternalString's data in its resource

For external uncached strings (also called "Small External Strings")
with cacheable resources, we can cache its resource's data at the
string's creation time. This allows us to safely read the data from the
background as we wouldn't trigger a data() callback.

For more information regarding the investigation and possible proposals
see
https://docs.google.com/document/d/101eAQqFpBPWFGNJicxtdlwYShJkTOUsEuxkVVeu5Hrk/edit?usp=sharing

Bug: v8:7790, v8:11463
Change-Id: I6164092b01a6ccb525a9516f476e066b35fb1f96
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2685177
Commit-Queue: Santiago Aboy Solanes <sol...@chromium.org>
Reviewed-by: Ulan Degenbaev <ul...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72862}

[modify] https://crrev.com/ed225df70cc62b03942850ddd5493c365aff3157/src/objects/string.h
[modify] https://crrev.com/ed225df70cc62b03942850ddd5493c365aff3157/src/objects/string-inl.h
[modify] https://crrev.com/ed225df70cc62b03942850ddd5493c365aff3157/src/api/api.cc
[modify] https://crrev.com/ed225df70cc62b03942850ddd5493c365aff3157/include/v8.h

bugdroid via monorail

unread,
Feb 19, 2021, 6:35:15 PM2/19/21
to v8-re...@googlegroups.com

Comment #3 on issue 11463 by bugdroid: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c3


The following revision refers to this bug:
https://chromium.googlesource.com/v8/v8/+/c2a00ed8045780c38d0f083bdb323c89ef766b4c

commit c2a00ed8045780c38d0f083bdb323c89ef766b4c
Author: Bill Budge <bbu...@chromium.org>
Date: Fri Feb 19 23:32:29 2021

Revert "[objects] Cache the ExternalString's data in its resource"

This reverts commit ed225df70cc62b03942850ddd5493c365aff3157.

Reason for revert: Blocks the roll, causing compile failures in Chromium:
https://ci.chromium.org/p/chromium/builders/try/win_chromium_compile_dbg_ng/800868?

Original change's description:

> [objects] Cache the ExternalString's data in its resource
>
> For external uncached strings (also called "Small External Strings")
> with cacheable resources, we can cache its resource's data at the
> string's creation time. This allows us to safely read the data from the
> background as we wouldn't trigger a data() callback.
>
> For more information regarding the investigation and possible proposals
> see
> https://docs.google.com/document/d/101eAQqFpBPWFGNJicxtdlwYShJkTOUsEuxkVVeu5Hrk/edit?usp=sharing
>
> Bug: v8:7790, v8:11463
> Change-Id: I6164092b01a6ccb525a9516f476e066b35fb1f96
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2685177
> Commit-Queue: Santiago Aboy Solanes <sol...@chromium.org>
> Reviewed-by: Ulan Degenbaev <ul...@chromium.org>
> Reviewed-by: Leszek Swirski <les...@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72862}

Bug: v8:7790
Bug: v8:11463
Change-Id: I1d14c2f9872d156d43d5d95c8a032a37ba9379cb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2708824
Auto-Submit: Bill Budge <bbu...@chromium.org>
Bot-Commit: Rubber Stamper <rubber-...@appspot.gserviceaccount.com>
Commit-Queue: Bill Budge <bbu...@chromium.org>
Reviewed-by: Bill Budge <bbu...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72880}

[modify] https://crrev.com/c2a00ed8045780c38d0f083bdb323c89ef766b4c/src/objects/string.h
[modify] https://crrev.com/c2a00ed8045780c38d0f083bdb323c89ef766b4c/src/objects/string-inl.h
[modify] https://crrev.com/c2a00ed8045780c38d0f083bdb323c89ef766b4c/src/api/api.cc
[modify] https://crrev.com/c2a00ed8045780c38d0f083bdb323c89ef766b4c/include/v8.h

bugdroid via monorail

unread,
Feb 23, 2021, 9:57:25 AM2/23/21
to v8-re...@googlegroups.com

Comment #4 on issue 11463 by bugdroid: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c4


The following revision refers to this bug:

Author: Santiago Aboy Solanes <sol...@chromium.org>
Date: Tue Feb 23 14:56:43 2021

Reland "[objects] Cache the ExternalString's data in its resource"

This is a reland of ed225df70cc62b03942850ddd5493c365aff3157

Reland changes: removed #if DEBUG from v8.h since it had compile errors
in chromium + windows. Also wasn't needed anyway since the method it was
calling was just a DCHECK.


Original change's description:
> [objects] Cache the ExternalString's data in its resource
>
> For external uncached strings (also called "Small External Strings")
> with cacheable resources, we can cache its resource's data at the
> string's creation time. This allows us to safely read the data from the
> background as we wouldn't trigger a data() callback.
>
> For more information regarding the investigation and possible proposals
> see
> https://docs.google.com/document/d/101eAQqFpBPWFGNJicxtdlwYShJkTOUsEuxkVVeu5Hrk/edit?usp=sharing
>
> Bug: v8:7790, v8:11463
> Change-Id: I6164092b01a6ccb525a9516f476e066b35fb1f96
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2685177
> Commit-Queue: Santiago Aboy Solanes <sol...@chromium.org>
> Reviewed-by: Ulan Degenbaev <ul...@chromium.org>
> Reviewed-by: Leszek Swirski <les...@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72862}

Bug: v8:7790
Bug: v8:11463
Change-Id: I7c8a54c814b92c8632fb0bcf5a33f57fec159443
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2710440

Reviewed-by: Ulan Degenbaev <ul...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>
Commit-Queue: Santiago Aboy Solanes <sol...@chromium.org>

bugdroid via monorail

unread,
Mar 1, 2021, 11:00:33 AM3/1/21
to v8-re...@googlegroups.com

Comment #5 on issue 11463 by bugdroid: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c5


The following revision refers to this bug:

Author: Santiago Aboy Solanes <sol...@chromium.org>
Date: Mon Mar 01 15:59:43 2021

[string] Add tests for uncached strings with cacheable resources

Add tests for internal external uncached strings with cacheable
resources, for the cached_data functionality added in
https://chromium-review.googlesource.com/c/v8/v8/+/2710440.


Bug: v8:7790, v8:11463
Change-Id: I679c50995d315cc4289452a00838b3cafa4c93e4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2715187

Reviewed-by: Leszek Swirski <les...@chromium.org>
Commit-Queue: Santiago Aboy Solanes <sol...@chromium.org>

sola… via monorail

unread,
Mar 19, 2021, 12:56:31 PM3/19/21
to v8-re...@googlegroups.com
Updates:
NextAction: 2021-06-19

Comment #6 on issue 11463 by sol...@chromium.org: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c6

Haven't seen any regressions so far. Setting a NextAction date to revisit in case we want to improve it further.

sola… via monorail

unread,
Jun 21, 2021, 4:46:13 AM6/21/21
to v8-re...@googlegroups.com
Updates:
Status: Fixed

Comment #8 on issue 11463 by sol...@google.com: ExternalString's data cache
https://bugs.chromium.org/p/v8/issues/detail?id=11463#c8

This has been working smoothly. Marking as fixed.
Reply all
Reply to author
Forward
0 new messages