Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925813002:
Make 512MB devices low end. (Closed)
DescriptionMake 512MB devices low end.
Devices reporting exactly 512MB are currently not categorized as low-end
thanks to a < instead of <=. Because of reserved amounts, physical 512MB
devices are reported as lower, so there isn't a known case where this makes
any difference, but this line of code has caused confusion a few times, so change
it for clarity's sake.
BUG=607311
Committed: https://crrev.com/0064e5ec6d8999d150a15ea657e26bc5b180bfa7
Cr-Commit-Position: refs/heads/master@{#390446}
Patch Set 1 #Patch Set 2 : Update the same check in sys_info.cc #
Messages
Total messages: 23 (10 generated)
mariakhomenko@chromium.org changed reviewers: + yfriedman@chromium.org
dskiba@google.com changed reviewers: + dskiba@google.com
lgtm
On 2016/04/27 21:26:47, Dmitry Skiba wrote: > lgtm uh: 1) yikes (and sorry!) 2) how did we not notice this? 3) how did you notice this?
On 2016/04/27 21:42:42, Yaron wrote: > On 2016/04/27 21:26:47, Dmitry Skiba wrote: > > lgtm > > uh: > 1) yikes (and sorry!) > 2) how did we not notice this? > 3) how did you notice this? doesn't sys_info.cc have the same issue (for non-android)
On 2016/04/27 21:45:18, Yaron wrote: > On 2016/04/27 21:42:42, Yaron wrote: > > On 2016/04/27 21:26:47, Dmitry Skiba wrote: > > > lgtm > > > > uh: > > 1) yikes (and sorry!) > > 2) how did we not notice this? > > 3) how did you notice this? There was a discussion today on chrome-memory-lon/browser-memory-mtv discussing the size of Svelte population. They were trying to estimate the size using the UMA we upload for isLowMemoryDevice(). That's when this got noticed. What happened was in 2014 we added a check that was using Activity.isLowRamDevice() with a fallback to ram check that used strictly less than. Then the strictly less than got fixed to <= in https://codereview.chromium.org/1261373002. However, when we removed the Activity-specific check, we used a rollback of an old CL, which reinstated the strictly less than check. > > doesn't sys_info.cc have the same issue (for non-android) I think it makes sense to keep them in sync, so I modified this as well.
mariakhomenko@chromium.org changed reviewers: + brettw@chromium.org
+ brettw for base/sys_info.cc approval. See previous comments for context.
lol lgtm
aelias@chromium.org changed reviewers: + aelias@chromium.org
I noticed I happened to be reintroducing this < when I rolled back, but I believe < vs <= doesn't make any difference. 512MB devices report somewhat less than 512MB from amountOfPhysicalMemoryMB, because this measures actually allocatable pages, and there is some amount reserved by the kernel and cell system. Check for yourself on a few 512MB devices.
On 2016/04/28 00:22:20, aelias wrote: > I noticed I happened to be reintroducing this < when I rolled back, but I > believe < vs <= doesn't make any difference. 512MB devices report somewhat less > than 512MB from amountOfPhysicalMemoryMB, because this measures actually > allocatable pages, and there is some amount reserved by the kernel and cell > system. Check for yourself on a few 512MB devices. lgtm Ya, that bug (https://bugs.chromium.org/p/chromium/issues/detail?id=515177) also mentions that this doesn't happen much in practice but seems like a reasonable change regardless.
Sounds fine, lgtm if only to avoid confusion.
Description was changed from ========== Make 512MB devices low end. BUG=607311 ========== to ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused some confusion. BUG=607311 ==========
Description was changed from ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused some confusion. BUG=607311 ========== to ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused confusion a few times, so change it if only for clarity's sake. BUG=607311 ==========
Description was changed from ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused confusion a few times, so change it if only for clarity's sake. BUG=607311 ========== to ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused confusion a few times, so change it for clarity's sake. BUG=607311 ==========
The CQ bit was checked by mariakhomenko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dskiba@google.com Link to the patchset: https://codereview.chromium.org/1925813002/#ps20001 (title: "Update the same check in sys_info.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925813002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925813002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0064e5ec6d8999d150a15ea657e26bc5b180bfa7 Cr-Commit-Position: refs/heads/master@{#390446}
Message was sent while issue was closed.
Description was changed from ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused confusion a few times, so change it for clarity's sake. BUG=607311 ========== to ========== Make 512MB devices low end. Devices reporting exactly 512MB are currently not categorized as low-end thanks to a < instead of <=. Because of reserved amounts, physical 512MB devices are reported as lower, so there isn't a known case where this makes any difference, but this line of code has caused confusion a few times, so change it for clarity's sake. BUG=607311 Committed: https://crrev.com/0064e5ec6d8999d150a15ea657e26bc5b180bfa7 Cr-Commit-Position: refs/heads/master@{#390446} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
