New issue
Advanced search Search tips

Issue 901815 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 5
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 3
Type: Bug

Blocking:
issue 877044



Sign in to add a comment

Memory.MovableStringParkingAction *Unparked* bucket proportion increased.

Project Member Reported by lizeb@chromium.org, Nov 5

Issue description

This is likely WAI, due to a reporting change.

# What's happening

## Previously

Previously, the code was:

$ git show 9aa76aede80b2e9317f02dccd00010ea2967d2c8:third_party/blink/renderer/platform/bindings/parkable_string_manager.cc | nl

    62  void ParkableStringManager::ParkAllIfRendererBackgrounded() {
[...]
    67    for (ParkableStringImpl* str : table_.Values()) {
    68      str->Park();
    69      total_size += str->CharactersSizeInBytes();
    70      count += 1;
    71    }


And in ParkableStringImpl:

   102  bool ParkableStringImpl::Park() {
[...]
   106    if (lock_depth_ != 0)
   107      return false;

   108    // Cannot park strings with several references.
   109    if (string_.Impl()->HasOneRef()) {
[...]
   115      RecordParkingAction(ParkingAction::kParkedInBackground);
   116      is_parked_ = true;
   117    }
   118    return is_parked_;
   119  }


So, the behavior is:
- Every time the renderer is backgrounded:
    1. Walk all parkable strings
    2. Report each non-locked, exclusively owned string as "parked in background"

Which means that a string which is parked and never unparked is reported once per transition to background.


## Now

$ git show e0caa0c7cb107e8b71fd5e67263e4dadafda66d4:third_party/blink/renderer/platform/bindings/parkable_string_manager.cc | nl
    89  void ParkableStringManager::ParkAllIfRendererBackgrounded() {
    90    DCHECK(IsMainThread());
    91    if (!IsRendererBackgrounded())
    92      return;
       
    93    if (!base::FeatureList::IsEnabled(kCompressParkableStringsInBackground))
    94      return;
       
    95    size_t total_size = 0;
    96    for (ParkableStringImpl* str : parked_strings_)
    97      total_size += str->CharactersSizeInBytes();
       
    98    for (ParkableStringImpl* str : unparked_strings_.Values()) {
    99      str->Park();  // Parking is asynchronous.
   100      total_size += str->CharactersSizeInBytes();
   101    }
[...]
   106  }

And
$ git show e0caa0c7cb107e8b71fd5e67263e4dadafda66d4:third_party/blink/renderer/platform/bindings/parkable_string.cc | nl


   172  bool ParkableStringImpl::Park() {                                                                                                                                                                                                                                                                                                                                  
   173    AssertOnValidThread();                                                                                                                                                                                                                                                                                                                                           
   174    MutexLocker locker(mutex_);                                                                                                                                                                                                                                                                                                                                      
   175    DCHECK(may_be_parked_);                                                                                                                                                                                                                                                                                                                                          
   176    if (state_ == State::kUnparked && CanParkNow()) {                                                                                                                                                                                                                                                                                                                
   177      // |string_|'s data should not be touched except in the compression task.                                                                                                                                                                                                                                                                                      
   178      AsanPoisonString(string_);                                                                                                                                                                                                                                                                                                                                     
   179      // |params| keeps |this| alive until |OnParkingCompleteOnMainThread()|.                                                                                                                                                                                                                                                                                        
   180      auto params = std::make_unique<CompressionTaskParams>(                                                                                                                                                                                                                                                                                                         
   181          this, string_.Bytes(), string_.CharactersSizeInBytes(),                                                                                                                                                                                                                                                                                                    
   182          Platform::Current()->CurrentThread()->GetTaskRunner());                                                                                                                                                                                                                                                                                                    
   183      background_scheduler::PostOnBackgroundThread(                                                                                                                                                                                                                                                                                                                  
   184          FROM_HERE, CrossThreadBind(&ParkableStringImpl::CompressInBackground,                                                                                                                                                                                                                                                                                      
   185                                     WTF::Passed(std::move(params))));                                                                                                                                                                                                                                                                                               
   186      state_ = State::kParkingInProgress;                                                                                                                                                                                                                                                                                                                            
   187    }    


   248  void ParkableStringImpl::OnParkingCompleteOnMainThread(                                                                                                                                                                                                                                                                                                            
   249      std::unique_ptr<CompressionTaskParams> params,                                                                                                                                                                                                                                                                                                                 
   250      std::unique_ptr<Vector<uint8_t>> compressed) {                                                                                                                                                                                                                                                                                                                 
   251    MutexLocker locker(mutex_);                                                                                                                                                                                                                                                                                                                                      
   252    DCHECK_EQ(State::kParkingInProgress, state_);                                                                                                                                                                                                                                                                                                                    
[...]
   264    if (CanParkNow() && compressed) {                                                                                                                                                                                                                                                                                                                                
   265      RecordParkingAction(ParkingAction::kParkedInBackground);                                                                                                                                                                                                                                                                                                       
   266      state_ = State::kParked;                                                                                                                                                                                                                                                                                                                                       
   267      ParkableStringManager::Instance().OnParked(this, string_.Impl());                                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                                                                           
   268      compressed_ = std::move(compressed);
   269      // Must unpoison the memory before releasing it.
   270      AsanUnpoisonString(string_);
   271      string_ = String();
   272    } else {
   273      state_ = State::kUnparked;
   274    }
   275  }

So, the new behavior is:
- Every time the renderer is backgrounded:
    1. Walk all *unparked* parkable strings
    2. Report each non-locked, exclusively owned string as "parked in background"

Meaning that now a string which never gets unparked is only reported once in the renderer lifetime.


# What that means

We are measuring different things:
- Are strings touched once the renderer is backgrounded? The previous measurement answers it better
- Are strings *ever* touched again after some point? The new measurement is somewhat better, though not ideal as a given string can be compressed several times (if the renderer is back/fore-grounded several times).




Status: WontFix (was: Started)

Sign in to add a comment