Abhishek Shanthkumar (Gerrit)

unread,

Oct 27, 2025, 9:34:08 AM (4 days ago) Oct 27

to Evan Stade, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Evan Stade

Abhishek Shanthkumar voted and added 3 comments

Votes added by Abhishek Shanthkumar

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Abhishek Shanthkumar . resolved

Nice!! This turned out to be relatively straightforward; perfect for MVP. Business logic LGTM (I'm not familiar with the idiosyncrasies if any in the snappy and zstd libraries).

File content/browser/indexed_db/instance/sqlite/database_connection.h

Line 353, Patchset 11 (Latest): // LINT.ThenChange(//tools/metrics/histograms/metadata/storage/enums.xml:IndexedDbSqliteSpecificEvent)

Abhishek Shanthkumar . unresolved

Don't we need to update enums.xml? Wonder why the lint didn't work.

File content/browser/indexed_db/instance/sqlite/database_connection.cc

Line 2250, Patchset 11 (Latest):

  return DoDecompress(compressed, compression_type)
      .or_else([&](Status status) -> StatusOr<std::vector<uint8_t>> {
        return base::unexpected(
            Fatal(status, SpecificEvent::kDecompressionFailure));
      });

Abhishek Shanthkumar . unresolved

`transform_error` is probably more appropriate here since we don't wish to call any function that returns a `base::expected`, i.e., this should work:

```suggestion
  return DoDecompress(compressed, compression_type)
      .transform_error([&](Status status){
        return Fatal(status, SpecificEvent::kDecompressionFailure);
      });
```

Open in Gerrit

Related details

Attention is currently required from:

Evan Stade

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Evan Stade (Gerrit)

unread,

Oct 27, 2025, 2:49:50 PM (4 days ago) Oct 27

to Mingyu Lei, Chromium Metrics Reviews, AyeAye, Abhishek Shanthkumar, Chromium LUCI CQ, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Abhishek Shanthkumar and Mingyu Lei

Evan Stade added 3 comments

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Evan Stade . resolved

Mingyu ptal metrics

File content/browser/indexed_db/instance/sqlite/database_connection.h

Line 353, Patchset 11: // LINT.ThenChange(//tools/metrics/histograms/metadata/storage/enums.xml:IndexedDbSqliteSpecificEvent)

Abhishek Shanthkumar . resolved

Don't we need to update enums.xml? Wonder why the lint didn't work.

Evan Stade

oh yeah, thanks for reminder. I believe the lint bot remarks are is only visible to Googlers, or something --- I did get pinged about a similar lint failure in the past by a Google metrics reviewer.

File content/browser/indexed_db/instance/sqlite/database_connection.cc

Line 2250, Patchset 11: return DoDecompress(compressed, compression_type)


      .or_else([&](Status status) -> StatusOr<std::vector<uint8_t>> {
        return base::unexpected(
            Fatal(status, SpecificEvent::kDecompressionFailure));
      });

Abhishek Shanthkumar . resolved

`transform_error` is probably more appropriate here since we don't wish to call any function that returns a `base::expected`, i.e., this should work:
```suggestion
  return DoDecompress(compressed, compression_type)
      .transform_error([&](Status status){
        return Fatal(status, SpecificEvent::kDecompressionFailure);
      });
```

Evan Stade

thanks!

Open in Gerrit

Related details

Attention is currently required from:

Abhishek Shanthkumar
Mingyu Lei

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Abhishek Shanthkumar (Gerrit)

unread,

Oct 28, 2025, 2:55:02 AM (3 days ago) Oct 28

to Evan Stade, Mingyu Lei, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Evan Stade and Mingyu Lei

Abhishek Shanthkumar voted and added 1 comment

Votes added by Abhishek Shanthkumar

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Abhishek Shanthkumar . resolved

SLGTM

Open in Gerrit

Related details

Attention is currently required from:

Evan Stade
Mingyu Lei

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mingyu Lei (Gerrit)

unread,

Oct 29, 2025, 5:05:57 AM (2 days ago) Oct 29

to Evan Stade, Abhishek Shanthkumar, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Attention needed from Evan Stade

Mingyu Lei voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Evan Stade

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Evan Stade (Gerrit)

unread,

Oct 29, 2025, 1:47:51 PM (2 days ago) Oct 29

to Mingyu Lei, Abhishek Shanthkumar, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Evan Stade voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Evan Stade (Gerrit)

unread,

Oct 29, 2025, 5:07:07 PM (2 days ago) Oct 29

to Mingyu Lei, Abhishek Shanthkumar, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Oct 29, 2025, 6:40:34 PM (2 days ago) Oct 29

to Evan Stade, Mingyu Lei, Abhishek Shanthkumar, Chromium Metrics Reviews, AyeAye, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, dmurph+wa...@chromium.org, dmurph+watching...@chromium.org, edgesto...@microsoft.com, enne...@chromium.org, extension...@chromium.org, ipc-securi...@chromium.org, kinuko...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

IDB: compress rows in SQLite store

Implements a straightforward compression of IDB values prior to storing
on disk. See doc linked in crbug for discussion.

Prior testing has shown ZSTD to have performance (time, compression
ratio) comparable to or slightly better than Snappy. ZSTD is the
preferred compression algorithm for this reason and because its
performance is tunable, in addition to having some more sophisticated
features of potential future use. We will be able to perform real-world
experiments with compression level and some other parameters (and also
compare to Snappy), but for now some reasonable defaults are assumed.

Snappy compression is also included in this patch as a fallback for
size-constrained platforms (Android, Fuchsia), which do not already
include ZSTD's compression library, although they do include
decompression. (In this change, the decompression code is included on
those platforms because it's cheap/easy to include, and on the off
chance someone copies data from a Chromium profile on a system that did
use ZSTD for compression to one of these devices).

Since the SQLite backing store is not live, this shouldn't affect real
users. We will soon start deploying the SQLite store to real users for
in-memory profiles (such as Incognito), which is an ideal testing ground
for compression performance.

Test coverage is provided by existing tests, and there will be more
specific tests added when we add the ability to tweak and monitor
behavior (for experimentation).

Bug: 436887362

Change-Id: I7c0691b224ed766df6c6de50b860376e6d589dfe

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7080983

Commit-Queue: Evan Stade <evan...@microsoft.com>

Reviewed-by: Abhishek Shanthkumar <abhishek.s...@microsoft.com>

Reviewed-by: Mingyu Lei <le...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1537606}

Files:

M content/browser/indexed_db/BUILD.gn
M content/browser/indexed_db/DEPS
M content/browser/indexed_db/instance/sqlite/database_connection.cc
M content/browser/indexed_db/instance/sqlite/database_connection.h
M tools/metrics/histograms/metadata/storage/enums.xml

Change size: M

Delta: 5 files changed, 148 insertions(+), 13 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Abhishek Shanthkumar, +1 by Mingyu Lei

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

IDB: compress rows in SQLite store [chromium/src : main]

Abhishek Shanthkumar (Gerrit)

Abhishek Shanthkumar voted and added 3 comments

Votes added by Abhishek Shanthkumar

3 comments

Related details

Evan Stade (Gerrit)

Evan Stade added 3 comments

Related details

Abhishek Shanthkumar (Gerrit)

Abhishek Shanthkumar voted and added 1 comment

Votes added by Abhishek Shanthkumar

1 comment

Related details

Mingyu Lei (Gerrit)

Mingyu Lei voted Code-Review+1

Related details

Evan Stade (Gerrit)

Evan Stade voted Commit-Queue+2

Related details

Evan Stade (Gerrit)

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information