|
|
DescriptionFix bug causing DCHECKs to be hit in DocumentMarkerController
These DCHECKs check that we're resetting the possibly_has_marker_types_ flag
when markers_ becomes empty. Resetting this flag properly enables a performance
optimization. These DCHECKs are causing sporadic crashes in debug builds; we
believe this is because the markers_ map is holding weak references to its keys
(Node objects), so it's possible for the map to become empty through garbage
collection, which doesn't reset possibly_has_marker_types_.
This CL modifies DocumentMarkerController::PossiblyHasMarkers() to catch this
case. Alternatively, we could handle this case at garbage collection time, but
we believe this approach has better performance characteristics.
BUG=685755
Review-Url: https://codereview.chromium.org/2891843003
Cr-Commit-Position: refs/heads/master@{#473400}
Committed: https://chromium.googlesource.com/chromium/src/+/37c906bd514195222ab091fa98a461cc40b25577
Patch Set 1 #
Total comments: 1
Patch Set 2 : Modify PossiblyHasMarkers() instead of removing DCHECKs #Messages
Total messages: 30 (13 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rlanday@chromium.org changed reviewers: + kouhei@chromium.org
kouhei: are we correctly understanding the behavior of HeapHashMap when used with WeakMember? It removes the keys from the map during GC if the objects the keys pointed at have been GCed, and this can cause the map to become empty?
https://codereview.chromium.org/2891843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2891843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:167: DCHECK(!markers_.IsEmpty()); Rather than removing DCHECK, how about changing PossiblyHasMarkers(..) to check markers_.IsEmpty()? For example: bool DocumentMarkerController::PossiblyHasMarkers( DocumentMarker::MarkerTypes types) { return !markes_.IsEmpty() && possibly_existing_marker_types_.Intersects(types); } In this way, we can get small patch. ;-)
Hmm...don't we still need to remove the DCHECKs though because they're checking for !markers_.IsEmpty(), and changing PossiblyHasMarkers() wouldn't affect the result of that check? If we want to modify PossiblyHasMarkers() to try to get a performance optimization for this case, I think we should do something like: if (markers_.IsEmpty()) possibly_existing_marker_types_ = 0; This way, if markers get added again, we can know we don't have any marker types other than the ones added since markers_ was empty (as opposed to whatever types might've been in there before). I'm not sure if adding this check is worth the performance optimization in this rare case or not.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/18 at 05:03:03, rlanday wrote: > Hmm...don't we still need to remove the DCHECKs though because they're checking for !markers_.IsEmpty(), and changing PossiblyHasMarkers() wouldn't affect the result of that check? > > If we want to modify PossiblyHasMarkers() to try to get a performance optimization for this case, I think we should do something like: > > if (markers_.IsEmpty()) > possibly_existing_marker_types_ = 0; > > This way, if markers get added again, we can know we don't have any marker types other than the ones added since markers_ was empty (as opposed to whatever types might've been in there before). I'm not sure if adding this check is worth the performance optimization in this rare case or not. markers_.IsEmpty() is fast enough, it is { return key_count_ == 0; }. If we want to reset |possibly_existingMarker_types_| to zero, we could write: bool DMC::PossiblyHasMarkers() const { if (possibly_existing_marker_types_ == 0) return false; if (!markes_.IsEmpty())) return true; possibly_existing_marker_types_ = 0; // mark mutable return false; } Since PossiblyHasMarkers() isn't used for painting. It is used during DOM modification, duration of PossiblyHasMarkers() doesn't use much time in whole DOM mutation time.
Ok, sounds good. On 2017/05/18 at 05:03:03, rlanday wrote: > Hmm...don't we still need to remove the DCHECKs though because they're checking for !markers_.IsEmpty(), and changing PossiblyHasMarkers() wouldn't affect the result of that check? I now realize that this is incorrect and we could actually leave the DCHECKs in place since we only check !markers_.IsEmpty() immediately after checking PossiblyHasMarkers()...I still think it doesn't make much sense to DCHECK for a condition that we're adding into PossiblyHasMarkers() though
On 2017/05/18 at 06:36:27, rlanday wrote: > Ok, sounds good. > > > On 2017/05/18 at 05:03:03, rlanday wrote: > > Hmm...don't we still need to remove the DCHECKs though because they're checking for !markers_.IsEmpty(), and changing PossiblyHasMarkers() wouldn't affect the result of that check? > > I now realize that this is incorrect and we could actually leave the DCHECKs in place since we only check !markers_.IsEmpty() immediately after checking PossiblyHasMarkers()...I still think it doesn't make much sense to DCHECK for a condition that we're adding into PossiblyHasMarkers() though Double checking is OK. Since each caller don't know what |PossiblyHasMarkers()| does. Checking |markers_.IsEmpty()| in |PossiblyHasMarkers()| increase possibility of return value of it == returns 100% sure false if there are no markers.
Discussed offline with yosin@. Feel free to ping me again if you have further problems.
yosin@chromium.org changed reviewers: - kouhei@chromium.org
Discussed offline with Oilpan team. For HashMap<WeakMember<T>, V>. GC remove entries if key is invalid. So, we have change |markers_.IsEmpty()| but |possibly_existing_marker_types_ != 0|. Note: This is done by Allocator::RegisterWeakMembers() and Allocator::RegisterWeakTable() in HashTable ctor. To prevent this, we should implement SynchronousMutationObserver::NodeWillBeRemoved() and NodeChildrenWillBeRemoved(); I'm not sure why we haven't done so, or I expected to do so.
On 2017/05/18 at 07:03:58, yosin_UTC9 wrote: > Discussed offline with Oilpan team. > > To prevent this, we should implement SynchronousMutationObserver::NodeWillBeRemoved() > and NodeChildrenWillBeRemoved(); I'm not sure why we haven't done so, or I expected to do so. We should NOT implement them. It increases execution time of Node#removeChild() and other Node removal functions. Hence, |possibly_existing_marker_types_| is really unreliable as its name suggested. I think early return from function when |!PossiblyHasMarkers(DocumentMarker::AllMarkers())| is better than removing |DCHECK(markers_.IsEmpty()|.
Description was changed from ========== Remove DCHECK(!markers_.IsEmpty())s in DocumentMarkerController These DCHECKs check that we're resetting the possibly_has_marker_types_ flag when markers_ becomes empty. Resetting this flag properly enables a performance optimization. These DCHECKs are causing sporadic crashes in debug builds; we believe this is because the markers_ map is holding weak references to its keys (Node objects), so it's possible for the map to become empty through garbage collection, which doesn't reset possibly_has_marker_types_. Losing the performance optimization in this case isn't a big issue, so the most straightforward solution is just to remove the DCHECKs. Note: I suspect we might be able to check if markers_ is empty in the Trace method and reset the flag there, but I'm not sure how wise it is to be trying to tack on extra code to run at garbage collection time. BUG=685755 ========== to ========== Fix bug causing DCHECKs to be hit in DocumentMarkerController These DCHECKs check that we're resetting the possibly_has_marker_types_ flag when markers_ becomes empty. Resetting this flag properly enables a performance optimization. These DCHECKs are causing sporadic crashes in debug builds; we believe this is because the markers_ map is holding weak references to its keys (Node objects), so it's possible for the map to become empty through garbage collection, which doesn't reset possibly_has_marker_types_. This CL modifies DocumentMarkerController::PossiblyHasMarkers() to catch this case. Alternatively, we could handle this case at garbage collection time, but we believe this approach has better performance characteristics. BUG=685755 ==========
Ok, I've updated the CL to leave the DCHECKs in place and modify PossiblyHasMarkers().
lgtm
lgtm
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495235442594080, "parent_rev": "44f776856864c18d20350b3e4c24844edcff26db", "commit_rev": "37c906bd514195222ab091fa98a461cc40b25577"}
Message was sent while issue was closed.
Description was changed from ========== Fix bug causing DCHECKs to be hit in DocumentMarkerController These DCHECKs check that we're resetting the possibly_has_marker_types_ flag when markers_ becomes empty. Resetting this flag properly enables a performance optimization. These DCHECKs are causing sporadic crashes in debug builds; we believe this is because the markers_ map is holding weak references to its keys (Node objects), so it's possible for the map to become empty through garbage collection, which doesn't reset possibly_has_marker_types_. This CL modifies DocumentMarkerController::PossiblyHasMarkers() to catch this case. Alternatively, we could handle this case at garbage collection time, but we believe this approach has better performance characteristics. BUG=685755 ========== to ========== Fix bug causing DCHECKs to be hit in DocumentMarkerController These DCHECKs check that we're resetting the possibly_has_marker_types_ flag when markers_ becomes empty. Resetting this flag properly enables a performance optimization. These DCHECKs are causing sporadic crashes in debug builds; we believe this is because the markers_ map is holding weak references to its keys (Node objects), so it's possible for the map to become empty through garbage collection, which doesn't reset possibly_has_marker_types_. This CL modifies DocumentMarkerController::PossiblyHasMarkers() to catch this case. Alternatively, we could handle this case at garbage collection time, but we believe this approach has better performance characteristics. BUG=685755 Review-Url: https://codereview.chromium.org/2891843003 Cr-Commit-Position: refs/heads/master@{#473400} Committed: https://chromium.googlesource.com/chromium/src/+/37c906bd514195222ab091fa98a4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/37c906bd514195222ab091fa98a4... |