Skip to content

Commit

Permalink
Revert "Deferred Shaping: Add a counter for tall IFCs" and "Deferred …
Browse files Browse the repository at this point in the history
…Shaping: Speculative fix for memory regressions"

This CL reverts crrev.com/931342 and crrev.com/931591.
It seems they consumed additional 400-700KB memory on each of pages.

Bug: 1259085, 1259877, 1259884, 1259873, 1259919, 1260075
Change-Id: Idae4dfd7e0c87a6ac996ec4c2a943adecab173f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3224759
Commit-Queue: Koji Ishii <kojii@chromium.org>
Auto-Submit: Kent Tamura <tkent@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#931863}
  • Loading branch information
tkent-google authored and Chromium LUCI CQ committed Oct 15, 2021
1 parent d016d62 commit f446da3
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 109 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3367,7 +3367,6 @@ enum WebFeature {
kFlexboxAlignSingleLineDifference = 4057,
kExternalProtocolBlockedBySandbox = 4058,
kWebAssemblyDynamicTiering = 4059,
kDeferredShapingTallerIfc = 4060,
kReadOrWriteWebDatabase = 4061,

// Add new features immediately above this line. Don't change assigned
Expand Down
100 changes: 0 additions & 100 deletions third_party/blink/renderer/core/dom/document.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@
#include "third_party/blink/renderer/bindings/core/v8/source_location.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_element_creation_options.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_element_registration_options.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_idle_request_options.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_interest_cohort.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_throw_dom_exception.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_union_elementcreationoptions_string.h"
Expand Down Expand Up @@ -416,97 +415,6 @@ bool DefaultFaviconAllowedByCSP(const Document* document, const IconURL& icon) {
ContentSecurityPolicy::CheckHeaderType::kCheckAll);
}

class IdleWebFeatureTask : public IdleTask {
public:
explicit IdleWebFeatureTask(Document& document)
: IdleTask(), document_(document) {}

void Trace(Visitor* visitor) const override {
visitor->Trace(document_);
IdleTask::Trace(visitor);
}

const char* NameInHeapSnapshot() const override {
return "WebPlatformFeatureStatisticsTask";
}

void invoke(IdleDeadline*) override {
Document* document = document_.Get();
if (!document || !document->View())
return;
if (layout_count_ != document->View()->LayoutCount()) {
layout_count_ = document->View()->LayoutCount();
if (!document->GetLayoutView())
return;
const LayoutView& view = *document->GetLayoutView();
PhysicalSize viewport_size(LayoutUnit(view.ViewWidth()),
LayoutUnit(view.ViewHeight()));
if (FindTallerIfc(view, viewport_size)) {
UseCounter::Count(*document, WebFeature::kDeferredShapingTallerIfc);
return;
}
if (first_traverse_time_.is_null())
first_traverse_time_ = base::Time::Now();
}
// Stop counting after kCountDuration from the first traverse.
constexpr base::TimeDelta kCountDuration = base::Seconds(30);
if (first_traverse_time_.is_null() ||
base::Time::Now() < first_traverse_time_ + kCountDuration)
document->RequestIdleCallback(this, IdleRequestOptions::Create());
}

private:
static bool FindTallerIfc(const LayoutBox& parent,
const PhysicalSize& viewport_size) {
for (const auto* child = parent.SlowFirstChild(); child;
child = child->NextSibling()) {
const auto* box = DynamicTo<LayoutBox>(child);
if (!box)
continue;
PhysicalSize new_size = viewport_size;
if (box->HasNonVisibleOverflow())
new_size = {box->Size().Width(), box->Size().Height()};
if (box->IsLayoutBlockFlow() && box->ChildrenInline() &&
!box->IsPositioned()) {
if (IsTallerBox(*box, new_size) && HasInlineOrNonWhitespaceText(*box))
return true;
}
if (FindTallerIfc(*box, new_size))
return true;
}
return false;
}

static bool IsTallerBox(const LayoutBox& box,
const PhysicalSize& viewport_size) {
// This function returns false for viewport block size == 0 because
// the counter is for "partial" deferred shaping.
if (box.IsHorizontalWritingMode()) {
return viewport_size.height > LayoutUnit() &&
box.Size().Height() > viewport_size.height;
}
return viewport_size.width > LayoutUnit() &&
box.Size().Width() > viewport_size.width;
}

static bool HasInlineOrNonWhitespaceText(const LayoutBox& ifc) {
for (const auto* child = ifc.SlowFirstChild(); child;
child = child->NextSibling()) {
if (child->IsLayoutInline())
return true;
if (const auto* text = DynamicTo<LayoutText>(child)) {
if (!text->ContainsOnlyWhitespace(0, text->TextLength()))
return true;
}
}
return false;
}

WeakMember<Document> document_;
base::Time first_traverse_time_;
uint32_t layout_count_ = 0;
};

} // namespace

static const unsigned kCMaxWriteRecursionDepth = 21;
Expand Down Expand Up @@ -2768,14 +2676,6 @@ void Document::Initialize() {

if (View())
View()->DidAttachDocument();

// We need to check IsRunningWebTest() and null Url(). Some tests fail
// without the check because of a scheduler behavior difference.
if (GetFrame()->IsLocalRoot() && !WebTestSupport::IsRunningWebTest() &&
!Url().IsNull()) {
RequestIdleCallback(MakeGarbageCollected<IdleWebFeatureTask>(*this),
IdleRequestOptions::Create());
}
}

void Document::Shutdown() {
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/renderer/core/frame/local_frame_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ LocalFrameView::LocalFrameView(LocalFrame& frame, IntRect frame_rect)
can_have_scrollbars_(true),
has_pending_layout_(false),
layout_scheduling_enabled_(true),
layout_count_(0),
layout_count_for_testing_(0),
lifecycle_update_count_for_testing_(0),
// We want plugin updates to happen in FIFO order with loading tasks.
update_plugins_timer_(frame.GetTaskRunner(TaskType::kInternalLoading),
Expand Down Expand Up @@ -1907,7 +1907,7 @@ void LocalFrameView::PerformPostLayoutTasks(bool visual_viewport_size_changed) {
TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(
TRACE_DISABLED_BY_DEFAULT("blink.debug.layout.trees"), "LayoutTree", this,
TracedLayoutObject::Create(*GetLayoutView(), true));
layout_count_++;
layout_count_for_testing_++;
Document* document = GetFrame().GetDocument();
DCHECK(document);
if (AXObjectCache* cache = document->ExistingAXObjectCache()) {
Expand Down
7 changes: 2 additions & 5 deletions third_party/blink/renderer/core/frame/local_frame_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,7 @@ class CORE_EXPORT LocalFrameView final
// Returns true if commits will be deferred for first contentful paint.
bool WillDoPaintHoldingForFCP() const;

// TODO(crbug.com/1259085): Remove LayoutCount() and rename layout_count_
// to layout_count_for_testing_ when we remove IdleWebFeatureTask.
unsigned LayoutCount() const { return layout_count_; }
unsigned LayoutCountForTesting() const { return layout_count_; }
unsigned LayoutCountForTesting() const { return layout_count_for_testing_; }
unsigned LifecycleUpdateCountForTesting() const {
return lifecycle_update_count_for_testing_;
}
Expand Down Expand Up @@ -1022,7 +1019,7 @@ class CORE_EXPORT LocalFrameView final
DepthOrderedLayoutObjectList orthogonal_writing_mode_root_list_;

bool layout_scheduling_enabled_;
unsigned layout_count_;
unsigned layout_count_for_testing_;
unsigned lifecycle_update_count_for_testing_;
HeapTaskRunnerTimer<LocalFrameView> update_plugins_timer_;

Expand Down
2 changes: 1 addition & 1 deletion tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34932,7 +34932,7 @@ Called by update_use_counter_feature_enum.py.-->
<int value="4057" label="FlexboxAlignSingleLineDifference"/>
<int value="4058" label="ExternalProtocolBlockedBySandbox"/>
<int value="4059" label="WebAssemblyDynamicTiering"/>
<int value="4060" label="DeferredShapingTallerIfc"/>
<int value="4060" label="OBSOLETE_DeferredShapingTallerIfc"/>
<int value="4061" label="ReadOrWriteWebDatabase"/>
</enum>

Expand Down

0 comments on commit f446da3

Please sign in to comment.