[RWM] Update MultiInstanceManagerApi31#allocInstanceId [chromium/src : main]
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aishwarya Rajesh
| Commit-Queue | +1 |
2 comments
if (instanceCount == mMaxInstances && UiUtils.isRobustWindowManagementEnabled()) {I'm a little wary of adding this check for all the allocation logic blocks in general; IMO, the only other time this may be useful is when we create new windows in quick succession using the Ctrl + N shortcut, that should ideally execute the default allocation logic. However, the default allocation logic is also a little more involved with a few checks and I'd rather do that separately and focus on handling race conditions that may arise from Ctrl + Shift + T (window restoration) for the purpose of this CL and the Recently Closed feature.
Related details
- Sirisha Kavuluru
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
Friendly bump - PTAL, thanks!
Related details
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 4 comments![]( "Open in Gerrit")
I am not completely clear on how this CL handles race condition. Adding my notes below on how the restoration flow works. Please correct me if I am wrong:
1. User hits ctrl+shift+T and activity 1 receives this key event
2. activity 1 triggers startActivity with a windowId
3. activity2 is starting to create. Steps in creation flow:
3.a: isStartedupCorrectly() is called which calls mMultiInstanceManager.allocInstanceId(). Returns an Id for activity2.
3.b: during inflation steps, createTabModel() is called which sets this Id in mMultiInstanceManager.initialize() which writes the Id to persistence store
If 2 ctrl+shift+T keyevents come in back to back:
step#2 above can occur in parallel - so 2 activity2 instances will attempt to be created. Each of this instance will have it's own object of MultiInstanceManager, correct? How would locking a local object help here?
synchronized (sAllocIdLock) {Apart from synchronized we could also use ReentrantReadWriteLock to allow concurrent reads but lock writes, but for this use-case synchronized probably makes more sense. Just flagging incase you want to evaluate
if (windowId >= 0 && instanceId == INVALID_WINDOW_ID) {Not related to this CL - can we rename windowId to maybe "preferredInstanceId" and instanceId to "instanceIdForTask" for consistency/readability
if (instanceCount == mMaxInstances && UiUtils.isRobustWindowManagementEnabled()) {I'm a little wary of adding this check for all the allocation logic blocks in general; IMO, the only other time this may be useful is when we create new windows in quick succession using the Ctrl + N shortcut, that should ideally execute the default allocation logic. However, the default allocation logic is also a little more involved with a few checks and I'd rather do that separately and focus on handling race conditions that may arise from Ctrl + Shift + T (window restoration) for the purpose of this CL and the Recently Closed feature.
>default allocation logic is also a little more involved with a few checks
Not clear on what the additional checks refers to - could we create a bug so we don't lose track of this additional edge case we might need to handle.
Related details
- Aishwarya Rajesh
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted and added 3 comments![]( "Open in Gerrit")
Votes added by Aishwarya Rajesh
| Commit-Queue | +1 |
3 comments
I am not completely clear on how this CL handles race condition. Adding my notes below on how the restoration flow works. Please correct me if I am wrong:
1. User hits ctrl+shift+T and activity 1 receives this key event
2. activity 1 triggers startActivity with a windowId
3. activity2 is starting to create. Steps in creation flow:
3.a: isStartedupCorrectly() is called which calls mMultiInstanceManager.allocInstanceId(). Returns an Id for activity2.
3.b: during inflation steps, createTabModel() is called which sets this Id in mMultiInstanceManager.initialize() which writes the Id to persistence storeIf 2 ctrl+shift+T keyevents come in back to back:
step#2 above can occur in parallel - so 2 activity2 instances will attempt to be created. Each of this instance will have it's own object of MultiInstanceManager, correct? How would locking a local object help here?
The restoration steps outlined above are correct - MIM#initialize() is what first writes lastAccessedTime, updates the task map etc.
How would locking a local object help here?
This is the reason I chose to use a static lock object (over a local lock) so it is shared by all MultiInstanceManager instances and the alloc process is synchronized across all to some extent.
The reason I say "to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized). In the meanwhile if we get another request to restore the same activity (because Ctrl + Shift + T would trigger a [#getAllInactiveInstances()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/RecentlyClosedEntriesManager.java;l=167) and the task map for this instance may not be updated yet, so this would still be considered inactive) and then we try allocating the same instanceId for the second created activity but because of `synchronized` I'm hoping it gives a little more delay for the second activity that is racing to get an id and eventually be assigned an invalid id because by this time it is already used up by the first triggered activity.
I did some exploratory testing and this was seeming to help smoothen the restoration experience but I think it's hard to guarantee this is objectively always the case or if this is necessary.
synchronized (sAllocIdLock) {Apart from synchronized we could also use ReentrantReadWriteLock to allow concurrent reads but lock writes, but for this use-case synchronized probably makes more sense. Just flagging incase you want to evaluate
Thanks for flagging!
I was thinking it would be good to disallow both concurrent reads/writes while allocating an id for an activity, but will consider evaluating as a next step (ie. while addressing general-purpose synchronization for this method for eg. CTRL + N races as mentioned in the other comment)
if (instanceCount == mMaxInstances && UiUtils.isRobustWindowManagementEnabled()) {Sirisha KavuluruI'm a little wary of adding this check for all the allocation logic blocks in general; IMO, the only other time this may be useful is when we create new windows in quick succession using the Ctrl + N shortcut, that should ideally execute the default allocation logic. However, the default allocation logic is also a little more involved with a few checks and I'd rather do that separately and focus on handling race conditions that may arise from Ctrl + Shift + T (window restoration) for the purpose of this CL and the Recently Closed feature.
>default allocation logic is also a little more involved with a few checks
Not clear on what the additional checks refers to - could we create a bug so we don't lose track of this additional edge case we might need to handle.
Not clear on what the additional checks refers to
The readability of this method is not very great at the moment; the default logic has checks to ignore an active task, compare lastAccessedTime to check recency of inactive instances, and supported profile type checks when incognito / regular windows come into play. This codepath could be critical even for non-app triggered flows for eg. launcher icon click to create a new activity and I'm not sure how an instance limit check would behave in this scenario (because there is a dependency on timely update of relevant persisted instance state). On the other hand, instance restoration is more or less controlled only by the app UI so it is safer to add this instance limit check in this scenario IMO.
All this is of course only an approximation / best-effort, and I'm yet to think more about how to guarantee a better id allocation behavior in general (I'm not even sure there's an optimal solution given the activity's async nature) but I thought adding `synchronized` + an additional instance limit check for instance restoration flows triggered from app UI is a good place to start handling races, which are likely to come mostly from keyboard shortcuts at the moment AFAICT.
crbug.com/438264585 is the other edge case I wish to look into, I've added it under the multi-instance refactor parent bug.
Related details
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru voted and added 4 comments![]( "Open in Gerrit")
Votes added by Sirisha Kavuluru
| Code-Review | +1 |
4 comments
Aishwarya RajeshI am not completely clear on how this CL handles race condition. Adding my notes below on how the restoration flow works. Please correct me if I am wrong:
1. User hits ctrl+shift+T and activity 1 receives this key event
2. activity 1 triggers startActivity with a windowId
3. activity2 is starting to create. Steps in creation flow:
3.a: isStartedupCorrectly() is called which calls mMultiInstanceManager.allocInstanceId(). Returns an Id for activity2.
3.b: during inflation steps, createTabModel() is called which sets this Id in mMultiInstanceManager.initialize() which writes the Id to persistence storeIf 2 ctrl+shift+T keyevents come in back to back:
step#2 above can occur in parallel - so 2 activity2 instances will attempt to be created. Each of this instance will have it's own object of MultiInstanceManager, correct? How would locking a local object help here?
The restoration steps outlined above are correct - MIM#initialize() is what first writes lastAccessedTime, updates the task map etc.
How would locking a local object help here?
This is the reason I chose to use a static lock object (over a local lock) so it is shared by all MultiInstanceManager instances and the alloc process is synchronized across all to some extent.
The reason I say "to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized). In the meanwhile if we get another request to restore the same activity (because Ctrl + Shift + T would trigger a [#getAllInactiveInstances()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/RecentlyClosedEntriesManager.java;l=167) and the task map for this instance may not be updated yet, so this would still be considered inactive) and then we try allocating the same instanceId for the second created activity but because of `synchronized` I'm hoping it gives a little more delay for the second activity that is racing to get an id and eventually be assigned an invalid id because by this time it is already used up by the first triggered activity.
I did some exploratory testing and this was seeming to help smoothen the restoration experience but I think it's hard to guarantee this is objectively always the case or if this is necessary.
"to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized).
Can you elaborate on this? Once activity received onCreate, steps after this should be sync. I think we trigger some of the startup steps in separate threads but does this involve instance Id alloca and persistence writes too?
I am also wondering if the mMultiInstanceManager.initialize() should be called post allocInstanceId instead of in createTabModel(). Why would we want to wait until tab model creation to write the allocated Id?
If we write immediately after allocation, we could lock both these steps together, so any new activity creation is guaranteed to read the latest value.
Added comment on current code structure - not blocking this CL but please evaluate for a follow up
synchronized (sAllocIdLock) {Aishwarya RajeshApart from synchronized we could also use ReentrantReadWriteLock to allow concurrent reads but lock writes, but for this use-case synchronized probably makes more sense. Just flagging incase you want to evaluate
Thanks for flagging!
I was thinking it would be good to disallow both concurrent reads/writes while allocating an id for an activity, but will consider evaluating as a next step (ie. while addressing general-purpose synchronization for this method for eg. CTRL + N races as mentioned in the other comment)
Acknowledged
if (instanceCount == mMaxInstances && UiUtils.isRobustWindowManagementEnabled()) {Sirisha KavuluruI'm a little wary of adding this check for all the allocation logic blocks in general; IMO, the only other time this may be useful is when we create new windows in quick succession using the Ctrl + N shortcut, that should ideally execute the default allocation logic. However, the default allocation logic is also a little more involved with a few checks and I'd rather do that separately and focus on handling race conditions that may arise from Ctrl + Shift + T (window restoration) for the purpose of this CL and the Recently Closed feature.
Aishwarya Rajesh>default allocation logic is also a little more involved with a few checks
Not clear on what the additional checks refers to - could we create a bug so we don't lose track of this additional edge case we might need to handle.
Not clear on what the additional checks refers to
The readability of this method is not very great at the moment; the default logic has checks to ignore an active task, compare lastAccessedTime to check recency of inactive instances, and supported profile type checks when incognito / regular windows come into play. This codepath could be critical even for non-app triggered flows for eg. launcher icon click to create a new activity and I'm not sure how an instance limit check would behave in this scenario (because there is a dependency on timely update of relevant persisted instance state). On the other hand, instance restoration is more or less controlled only by the app UI so it is safer to add this instance limit check in this scenario IMO.
All this is of course only an approximation / best-effort, and I'm yet to think more about how to guarantee a better id allocation behavior in general (I'm not even sure there's an optimal solution given the activity's async nature) but I thought adding `synchronized` + an additional instance limit check for instance restoration flows triggered from app UI is a good place to start handling races, which are likely to come mostly from keyboard shortcuts at the moment AFAICT.
crbug.com/438264585 is the other edge case I wish to look into, I've added it under the multi-instance refactor parent bug.
- Aishwarya Rajesh
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 3 comments![]( "Open in Gerrit")
Aishwarya RajeshI am not completely clear on how this CL handles race condition. Adding my notes below on how the restoration flow works. Please correct me if I am wrong:
1. User hits ctrl+shift+T and activity 1 receives this key event
2. activity 1 triggers startActivity with a windowId
3. activity2 is starting to create. Steps in creation flow:
3.a: isStartedupCorrectly() is called which calls mMultiInstanceManager.allocInstanceId(). Returns an Id for activity2.
3.b: during inflation steps, createTabModel() is called which sets this Id in mMultiInstanceManager.initialize() which writes the Id to persistence storeIf 2 ctrl+shift+T keyevents come in back to back:
step#2 above can occur in parallel - so 2 activity2 instances will attempt to be created. Each of this instance will have it's own object of MultiInstanceManager, correct? How would locking a local object help here?
Sirisha KavuluruThe restoration steps outlined above are correct - MIM#initialize() is what first writes lastAccessedTime, updates the task map etc.
How would locking a local object help here?
This is the reason I chose to use a static lock object (over a local lock) so it is shared by all MultiInstanceManager instances and the alloc process is synchronized across all to some extent.
The reason I say "to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized). In the meanwhile if we get another request to restore the same activity (because Ctrl + Shift + T would trigger a [#getAllInactiveInstances()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/RecentlyClosedEntriesManager.java;l=167) and the task map for this instance may not be updated yet, so this would still be considered inactive) and then we try allocating the same instanceId for the second created activity but because of `synchronized` I'm hoping it gives a little more delay for the second activity that is racing to get an id and eventually be assigned an invalid id because by this time it is already used up by the first triggered activity.
I did some exploratory testing and this was seeming to help smoothen the restoration experience but I think it's hard to guarantee this is objectively always the case or if this is necessary.
"to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized).Can you elaborate on this? Once activity received onCreate, steps after this should be sync. I think we trigger some of the startup steps in separate threads but does this involve instance Id alloca and persistence writes too?
I am also wondering if the mMultiInstanceManager.initialize() should be called post allocInstanceId instead of in createTabModel(). Why would we want to wait until tab model creation to write the allocated Id?
If we write immediately after allocation, we could lock both these steps together, so any new activity creation is guaranteed to read the latest value.
Once activity received onCreate, steps after this should be sync. I think we trigger some of the startup steps in separate threads but does this involve instance Id alloca and persistence writes too
Hmm true, I think you're right. I think #initialize() (and subsequent tab model observation to update tab count / incognito state etc. which I think is also on the ui thread right?) are the last instance state writes that happen after startup.
wondering if the mMultiInstanceManager.initialize() should be called post allocInstanceId instead of in createTabModel()
I can't say for sure but I'm assuming this was done to ensure we always have valid tab info (title, url, count, incognito state) for all persisted instances (or at least have an observer to that consumes updates to the tabs after initialization). Also post id allocation we might still finish the activity if id assigned = -1. I think we might be able to update task map + last accessed time at the very least if allocated id is valid, I've added a comment to [this bug]() to evaluate this possibility to improve the alloc id synchronization.
Added comment on current code structure - not blocking this CL but please evaluate for a follow up
Is this referring to https://chromium-review.googlesource.com/c/chromium/src/+/7409604/comments/fe8072e7_d183cbc3? I've linked the thread to the follow-up bug in that case.
if (windowId >= 0 && instanceId == INVALID_WINDOW_ID) {Not related to this CL - can we rename windowId to maybe "preferredInstanceId" and instanceId to "instanceIdForTask" for consistency/readability
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sirisha Kavuluru
| Code-Review | +1 |
Aishwarya RajeshI am not completely clear on how this CL handles race condition. Adding my notes below on how the restoration flow works. Please correct me if I am wrong:
1. User hits ctrl+shift+T and activity 1 receives this key event
2. activity 1 triggers startActivity with a windowId
3. activity2 is starting to create. Steps in creation flow:
3.a: isStartedupCorrectly() is called which calls mMultiInstanceManager.allocInstanceId(). Returns an Id for activity2.
3.b: during inflation steps, createTabModel() is called which sets this Id in mMultiInstanceManager.initialize() which writes the Id to persistence storeIf 2 ctrl+shift+T keyevents come in back to back:
step#2 above can occur in parallel - so 2 activity2 instances will attempt to be created. Each of this instance will have it's own object of MultiInstanceManager, correct? How would locking a local object help here?
Sirisha KavuluruThe restoration steps outlined above are correct - MIM#initialize() is what first writes lastAccessedTime, updates the task map etc.
How would locking a local object help here?
This is the reason I chose to use a static lock object (over a local lock) so it is shared by all MultiInstanceManager instances and the alloc process is synchronized across all to some extent.
The reason I say "to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized). In the meanwhile if we get another request to restore the same activity (because Ctrl + Shift + T would trigger a [#getAllInactiveInstances()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/RecentlyClosedEntriesManager.java;l=167) and the task map for this instance may not be updated yet, so this would still be considered inactive) and then we try allocating the same instanceId for the second created activity but because of `synchronized` I'm hoping it gives a little more delay for the second activity that is racing to get an id and eventually be assigned an invalid id because by this time it is already used up by the first triggered activity.
I did some exploratory testing and this was seeming to help smoothen the restoration experience but I think it's hard to guarantee this is objectively always the case or if this is necessary.
Aishwarya Rajesh"to some extent" is because activity startup itself is asynchronous (even after id allocation, up till the activity is fully initialized).Can you elaborate on this? Once activity received onCreate, steps after this should be sync. I think we trigger some of the startup steps in separate threads but does this involve instance Id alloca and persistence writes too?
I am also wondering if the mMultiInstanceManager.initialize() should be called post allocInstanceId instead of in createTabModel(). Why would we want to wait until tab model creation to write the allocated Id?
If we write immediately after allocation, we could lock both these steps together, so any new activity creation is guaranteed to read the latest value.
Once activity received onCreate, steps after this should be sync. I think we trigger some of the startup steps in separate threads but does this involve instance Id alloca and persistence writes too
Hmm true, I think you're right. I think #initialize() (and subsequent tab model observation to update tab count / incognito state etc. which I think is also on the ui thread right?) are the last instance state writes that happen after startup.
wondering if the mMultiInstanceManager.initialize() should be called post allocInstanceId instead of in createTabModel()
I can't say for sure but I'm assuming this was done to ensure we always have valid tab info (title, url, count, incognito state) for all persisted instances (or at least have an observer to that consumes updates to the tabs after initialization). Also post id allocation we might still finish the activity if id assigned = -1. I think we might be able to update task map + last accessed time at the very least if allocated id is valid, I've added a comment to [this bug]() to evaluate this possibility to improve the alloc id synchronization.
Thanks Ash - improving the alloc login in another bug sgtm
Related details
- Aishwarya Rajesh
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[RWM] Update MultiInstanceManagerApi31#allocInstanceId
This CL adds some minor improvements to allocInstanceId() to ensure a
potentially smoother window creation behavior at / near instance limit:
1. Synchronize allocation logic.
2. Check instance count to determine if the newly created activity needs
to be finished due to the instance limit.
- M chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiInstanceManagerApi31.java
Code-Review: +1 by Sirisha Kavuluru
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |