New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 854493 link

Starred by 2 users
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Sep 27
Cc:
mastiz@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 851905


Sign in to add a comment

Avoid sending unnecessary SESSIONS updates for placeholder tabs

Project Member Reported by mastiz@chromium.org, Jun 20 2018 
This is about a non-user-facing optimization that should reduce traffic to sync servers and reduce data usage, for Android users.

Recent changes in  crbug.com/851905  cause tab IDs to be stable across restarts on Android, which gets reflected in the sync protocol as |tab_id| protobuf field for both header and tab entities.

For placeholder tabs (i.e. tabs that exist on Android but haven't been fully loaded, i.e. no WebContents), we have been sending regular updates because the tab ID could have changed, and it was important to keep this in sync with the header's content.

Because tab IDs don't change, there's only one remaining field that we update for placeholder tabs: the window ID.

Sending commits for the sole purpose of updating window IDs is wasteful, because nothing really makes use of that information. The tab/window hierarchy (i.e. calls to PutWindowInSession() and PutTabInWindow()) is built based on the header entity only.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 20 2018 

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

commit 5651a8f2706b48abd0cf81ec187a9b262376bce7
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jun 20 11:08:05 2018

Avoiding sending placeholder updates if window ID doesn't change

Sending the very same tab entity is wasteful: in most cases it will be
caught by other layers (which will realize the proto is identical) but
let's detect it in early stages, which also avoids unnecessary I/O to
disk.

Bug:  854493 
Change-Id: Ia2ceaa4347bdf4dca1c5d0995b2466d07b41261a
Reviewed-on: https://chromium-review.googlesource.com/1107060
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568788}
[modify] https://crrev.com/5651a8f2706b48abd0cf81ec187a9b262376bce7/components/sync_sessions/local_session_event_handler_impl.cc
[modify] https://crrev.com/5651a8f2706b48abd0cf81ec187a9b262376bce7/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/5651a8f2706b48abd0cf81ec187a9b262376bce7/components/sync_sessions/sessions_sync_manager_unittest.cc

Comment 2 by mastiz@chromium.org, Jul 30 

I'll go ahead and land the last patch.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 30 

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

commit f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Jul 30 11:07:20 2018

Don't sync unnecessary updates for placeholder tabs

Recent changes in  crbug.com/851905  cause tab IDs to be stable across
restarts on Android. This means there's only one remaining field that we
update for placeholder tabs: the window ID.

Nodoby really cares about the window ID field in tab entities, because
the window/tab hierarchy is constructed using the header entity, where
tab IDs are used as foreign keys to refer to tabs (independently of
window IDs).

Hence, let's avoid wasting data use and traffic to sync servers by
simply letting the window IDs become stale in tab entities.

Bug:  854493 
Change-Id: I65b054d564fd47b4664d6376fda0f7ff799d259d
Reviewed-on: https://chromium-review.googlesource.com/1107989
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579009}
[modify] https://crrev.com/f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b/components/sync_sessions/local_session_event_handler_impl.cc
[modify] https://crrev.com/f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b/components/sync_sessions/local_session_event_handler_impl.h
[modify] https://crrev.com/f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b/components/sync_sessions/local_session_event_handler_impl_unittest.cc
[modify] https://crrev.com/f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/f2fefd3a1a0fc41e0654f69d8ee01ca35cd0105b/components/sync_sessions/sessions_sync_manager_unittest.cc

Comment 4 by melandory@chromium.org, Sep 9 

sync-triage-ping, any updates?

Was the bug fixed in #3?

Comment 5 by mastiz@chromium.org, Sep 27

Status: Fixed (was: Assigned)
Right, this is fixed and seems to reduce commit traffic in about 30% on Clank!
►

Sign in to add a comment