All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=747028

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=79b60a9efefa20882e0ba59fd9c17c24a5843eef7bbae822fe5a447c99b15a45


Bot(s) for this bug's original alert(s):

android-webview-nexus5X

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8973531173181587408

=== Auto-CCing suspected CL author adenilson.cavalcanti@arm.com ===

Hi adenilson.cavalcanti@arm.com, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Adenilson Cavalcanti
  Commit : cb4e97648309bafb70ee7aa725d39b6599d6011e
  Date   : Tue Jul 18 18:59:45 2017
  Subject: Specialized character type constructors to improve ShapeCache hashing

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:webview:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/background/after_https_mobile_twitter_com_justinbieber_skip_interstitial_true
  Change       : 5.52% | 54042589.3333 -> 57026525.3333

Revision             Result                   N
chromium@487499      54042589 +- 1135252      6      good
chromium@487531      54098227 +- 731953       6      good
chromium@487539      54041907 +- 778339       6      good
chromium@487541      54281523 +- 1229492      6      good
chromium@487542      54151645 +- 704575       6      good
chromium@487543      57265117 +- 617044       6      bad       <--
chromium@487547      57267165 +- 622017       6      bad
chromium@487563      57026525 +- 440810       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests memory.top_10_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8973531173181587408


For feedback, file a bug with component Speed>Bisection

Are there ways for your CL to use less memory? Can you please look into this this week as we may need to revert your CL to maintain the level of performance we want.

Issue 746719 has been merged into this issue.

It is by design to use more memory *if* we pre-allocate more space in the HashTable used by the word shape cache.

It is basically a trade-off between memory use and the need to re-hash the table for each new inclusion. Re-hashes are expensive O(n) operations and should be avoided.

What I would like to know is what would be acceptable limits?

An easy way to mitigate this issue would be to simply have a smaller initial size reserved,  currently is about 5% of the maximum word shape cache (i.e. 10,000 words, therefore 500 elements).

I can push a patch restricting it to only 100 elements.

If we cannot use a bit more memory (with the cost at a bit longer page loading), one alternative is simply to remove the code at:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/shaping/ShapeCache.h?q=ShapeCache&sq=package:chromium&l=116

Let me know which course you prefer to do in this matter.

Adding Emil as he has deeper understanding of the word shape cache.

Please see an initial fix (should reduce memory use in 5x):
https://chromium-review.googlesource.com/c/582508/

Issue 747010 has been merged into this issue.

Another alternative fix (simply remove the pre-allocation):
https://chromium-review.googlesource.com/c/582489/

The funny thing is that I'm about to leave on vacations (as a matter of fact starting today... should be back in 2 weeks).

So I guess we could immediately land this one to fix the regression and when I'm back experiment with the patch setting the size to 100 elements?

Let me know what you prefer to do.

A quick look on the sizes of each element in the hashmap shows:
### Sizeofs. SmallStringKey: 36	ShapeCacheEntry: 8	SmallStringKeyHash: 1	SmallStringKeyHashTraits: 1

Times 500 = 23000 bytes.

I'm unsure how much HashMap/HashTable would use per entry but yeah, it is far away from 4MB.

Therefore, feel free to revert the patch and I can do further investigations when I'm back.

I sent a revert to CQ

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b0a3c9b588ba16318dea3a7704bdde8662a7b3c

commit 2b0a3c9b588ba16318dea3a7704bdde8662a7b3c
Author: Annie Sullivan <sullivan@chromium.org>
Date: Mon Jul 24 16:15:28 2017

BUG= 747028 


Revert "Specialized character type constructors to improve ShapeCache hashing"

This reverts commit cb4e97648309bafb70ee7aa725d39b6599d6011e.

Reason for revert: Reverting per comments on  bug 747028 , it's a memory regression and we'll revert during investigation.

Original change's description:
> Specialized character type constructors to improve ShapeCache hashing
> 
> This allows to avoid copying text runs character-per-character
> for 16bits text.
> 
> Also there is no need to hash per character, just hash the
> whole string in one go. This also helps to save the construction
> of a StringHasher object (as we use the static method to hash
> the strings).
> 
> Finally, reserve in HashTable space for 500 words (this reduces
> the number of rehashes in up to 4x).
> 
> Bug:  735674 
> Change-Id: I4c5e23bb1e21dad995d4948fae0943a2b01309e0
> Reviewed-on: https://chromium-review.googlesource.com/564076
> Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Commit-Queue: Adenilson Cavalcanti <cavalcantii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487543}

TBR=thakis@chromium.org,eae@chromium.org,drott@chromium.org,cavalcantii@chromium.org,adenilson.cavalcanti@arm.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  735674 
Change-Id: I5b8efd92dcbdd03a3f6b07973c80e9f0ffbc7f93
Reviewed-on: https://chromium-review.googlesource.com/583127
Reviewed-by: Annie Sullivan <sullivan@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Annie Sullivan <sullivan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488989}
[modify] https://crrev.com/2b0a3c9b588ba16318dea3a7704bdde8662a7b3c/third_party/WebKit/Source/platform/BUILD.gn
[delete] https://crrev.com/434d4dfe55708db7f54d7820626fcf5de0d0b63c/third_party/WebKit/Source/platform/fonts/shaping/ShapeCache.cpp
[modify] https://crrev.com/2b0a3c9b588ba16318dea3a7704bdde8662a7b3c/third_party/WebKit/Source/platform/fonts/shaping/ShapeCache.h

+primiano to answer questions in comment #7.

#7 is saying that this is a deliberate tradeoff between memory and perf, which looks sensible.

My only question is: where is the (non-memory) benchmark that is improving as a result of this tradeoff?
My hope is that we are not regressing something that we measure (memory) to improve something that we don't measure (the perf benefit of the word shape cache).
The risk in that case is that if, some months from now, somebody regresses the latter we won't realize it and that would be a lose(memory)-lose(perf).

Issue 747915 has been merged into this issue.

Closing the issue as the memory issue was fixed.