Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217833003:
Fix for issue 25954. (Closed)
DescriptionFix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0).
out/ReleaseX64/dart ~/tmp/gc2.dart 1
permanent = 44739242 ephemeral = 671088620 21.875s
out/ReleaseX64/dart ~/tmp/gc2.dart 2
permanent = 89478485 ephemeral = 1342177280 45.3s
out/ReleaseX64/dart ~/tmp/gc2.dart 3
permanent = 134217728 ephemeral = 2013265920 79.41s
out/ReleaseX64/dart ~/tmp/gc2.dart 4
permanent = 178956970 ephemeral = 2684354540 101.525s
BUG=25954
R=zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/85cb1b3bd82f8f860e9a5aba15796f7c727b5a81
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address code review comments. #Patch Set 3 : Address code review comments. #
Total comments: 10
Patch Set 4 : Address code review comments. #Patch Set 5 : Adjust comments. #Patch Set 6 : Tweak the heap growth a bit for the case when there is garbage and we decide to grow by heap_growth… #Messages
Total messages: 11 (4 generated)
Description was changed from ========== Fix for issue 25954. out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 18.059s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 33.287s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 58.452s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 76.187s BUG=25954 ========== to ========== Fix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0). out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 18.059s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 33.287s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 58.452s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 76.187s BUG=25954 ==========
asiva@google.com changed reviewers: + zra@google.com
I think I might need some help understanding how this works. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc File runtime/vm/pages.cc (right): https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1160: int gc_time_fraction = history_.GarbageCollectionTimeFraction(); const intptr_t https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1165: intptr_t allocated_since_previous_gc = const ASSERT(allocated_since_previous_gc > 0); https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1167: intptr_t garbage = before.used_in_words - after.used_in_words; const ASSERT(garbage >= 0); https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1168: double k = garbage / static_cast<double>(allocated_since_previous_gc); const https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1169: int garbage_ratio = static_cast<int>(k * 100); const intptr_t https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1183: intptr_t grow_ratio = const https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1187: grow_heap_ = Utils::Maximum(grow_heap_, grow_ratio); grow_heap_ = Utils::Maximum(heap_growth_max_, grow_ratio); https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1194: // Find minimum 'grow_heap_' such that after increasing capacity by Duplicated comment https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1199: intptr_t limit = const https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1201: intptr_t allocated_before_next_gc = limit - after.used_in_words; const https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1202: double estimated_garbage = k * allocated_before_next_gc; const https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1216: intptr_t freed_pages = const
https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc File runtime/vm/pages.cc (right): https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1160: int gc_time_fraction = history_.GarbageCollectionTimeFraction(); On 2016/08/08 14:59:37, zra wrote: > const intptr_t Added const but changing it to intptr_t would mean having to change the return type of GarbageCollectionTimeFraction. Would probably change that if needed in a different cl. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1165: intptr_t allocated_since_previous_gc = On 2016/08/08 14:59:37, zra wrote: > const > > ASSERT(allocated_since_previous_gc > 0); Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1167: intptr_t garbage = before.used_in_words - after.used_in_words; On 2016/08/08 14:59:37, zra wrote: > const > > ASSERT(garbage >= 0); Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1168: double k = garbage / static_cast<double>(allocated_since_previous_gc); On 2016/08/08 14:59:37, zra wrote: > const Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1169: int garbage_ratio = static_cast<int>(k * 100); On 2016/08/08 14:59:37, zra wrote: > const intptr_t Added const, change to intptr_t would be coupled with the change above if needed. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1183: intptr_t grow_ratio = On 2016/08/08 14:59:37, zra wrote: > const Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1187: grow_heap_ = Utils::Maximum(grow_heap_, grow_ratio); On 2016/08/08 14:59:37, zra wrote: > grow_heap_ = Utils::Maximum(heap_growth_max_, grow_ratio); Done but had to introduce a static_cast as heap_growth_max_ is an 'int' https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1194: // Find minimum 'grow_heap_' such that after increasing capacity by On 2016/08/08 14:59:37, zra wrote: > Duplicated comment Removed duplicate. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1199: intptr_t limit = On 2016/08/08 14:59:37, zra wrote: > const Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1201: intptr_t allocated_before_next_gc = limit - after.used_in_words; On 2016/08/08 14:59:37, zra wrote: > const Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1202: double estimated_garbage = k * allocated_before_next_gc; On 2016/08/08 14:59:37, zra wrote: > const Done. https://codereview.chromium.org/2217833003/diff/1/runtime/vm/pages.cc#newcode... runtime/vm/pages.cc:1216: intptr_t freed_pages = On 2016/08/08 14:59:37, zra wrote: > const Done.
https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc File runtime/vm/pages.cc (right): https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1181: if (garbage_ratio == 0) { Why 0? Why not e.g. <= 5? Maybe add TODO to investigate. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1187: - after.capacity_in_words) / PageSpace::kPageSizeInWords; There are two '-'. One at the end of the previous line and one at the beginning of this line. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1196: grow_heap_ = 0; Maybe use a local, and then write the field at the end. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1211: grow_heap_ += adjustment; Why is this adjustment needed? If the search results in grow_heap_ = 0, couldn't this adjustment make it negative? https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1215: OS::PrintErr("grow_heap_ = %" Pd "\n", grow_heap_); Leftover debug print?
Description was changed from ========== Fix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0). out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 18.059s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 33.287s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 58.452s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 76.187s BUG=25954 ========== to ========== Fix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0). out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 21.875s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 45.3s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 79.41s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 101.525s BUG=25954 ==========
https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc File runtime/vm/pages.cc (right): https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1181: if (garbage_ratio == 0) { On 2016/08/11 18:29:15, zra wrote: > Why 0? Why not e.g. <= 5? Maybe add TODO to investigate. I did not want to change the growth pattern when garbage is present even if small as it does not establish a pattern of growth. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1187: - after.capacity_in_words) / PageSpace::kPageSizeInWords; On 2016/08/11 18:29:16, zra wrote: > There are two '-'. One at the end of the previous line and one at the beginning > of this line. Good catch. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1196: grow_heap_ = 0; On 2016/08/11 18:29:15, zra wrote: > Maybe use a local, and then write the field at the end. Done. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1211: grow_heap_ += adjustment; On 2016/08/11 18:29:16, zra wrote: > Why is this adjustment needed? If the search results in grow_heap_ = 0, couldn't > this adjustment make it negative? I added the adjustment to keep it in sync with the old computation, we always set it to a positive value so it won't become negative, I have added an ASSERT to ensure grow_heap_ is always positive or 0. https://codereview.chromium.org/2217833003/diff/40001/runtime/vm/pages.cc#new... runtime/vm/pages.cc:1215: OS::PrintErr("grow_heap_ = %" Pd "\n", grow_heap_); On 2016/08/11 18:29:16, zra wrote: > Leftover debug print? Removed.
lgtm
Description was changed from ========== Fix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0). out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 21.875s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 45.3s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 79.41s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 101.525s BUG=25954 ========== to ========== Fix for issue 25954 (Adjust heap growth amount for the case when the amount of garbage collected in the previous round is 0). out/ReleaseX64/dart ~/tmp/gc2.dart 1 permanent = 44739242 ephemeral = 671088620 21.875s out/ReleaseX64/dart ~/tmp/gc2.dart 2 permanent = 89478485 ephemeral = 1342177280 45.3s out/ReleaseX64/dart ~/tmp/gc2.dart 3 permanent = 134217728 ephemeral = 2013265920 79.41s out/ReleaseX64/dart ~/tmp/gc2.dart 4 permanent = 178956970 ephemeral = 2684354540 101.525s BUG=25954 R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/85cb1b3bd82f8f860e9a5aba15796f7c727b5a81 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 85cb1b3bd82f8f860e9a5aba15796f7c727b5a81 (presubmit successful). |
