Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12223064:
base: Fix parsing and add dalvik-heap-limit (Closed)
|
Created:
7 years, 10 months ago by epenner Modified:
7 years, 10 months ago Reviewers:
gman1, nduca, jar (doing other things), ccameron, apatrick_chromium, piman, klobag.chromium CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, apatrick_chromium, wtc Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis fixes the overflows identified in the parsing
function, and adds the dalvik-heap-limit.
BUG=176201
NOTRY=true
No try since it has passed many times and is taking days in the CQ.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182986
Patch Set 1 #Patch Set 2 : Fix unit test. #Patch Set 3 : Fix tests. #
Total comments: 2
Patch Set 4 : Remove Android specific path. #
Total comments: 2
Patch Set 5 : Address feedback, #
Total comments: 19
Patch Set 6 : Reduce scope. #Patch Set 7 : CHECK overflow of int64. #Patch Set 8 : #Patch Set 9 : Address feedback. #Patch Set 10 : Rebase. #Patch Set 11 : Rebase. #Patch Set 12 : Remove CHECKs. Return error instead. #Messages
Total messages: 29 (0 generated)
Ptal. Unfortunately this combined with ccameron's last patch still doesn't allocate us the full limit. But in any case I think we want this change.
Perhaps instead you could override GLContext::GetTotalGpuMemory for android (in gl_context_android.cc) instead? That way, the Davlik heap info would stay out of gpu_memory_manager.cc, and we would only have one path kept around. https://codereview.chromium.org/12223064/diff/6001/content/common/gpu/gpu_mem... File content/common/gpu/gpu_memory_manager.cc (right): https://codereview.chromium.org/12223064/diff/6001/content/common/gpu/gpu_mem... content/common/gpu/gpu_memory_manager.cc:210: int max_surface_area = 0; Can kill off max_surface_area here. https://codereview.chromium.org/12223064/diff/6001/content/common/gpu/gpu_mem... File content/common/gpu/gpu_memory_manager.h (right): https://codereview.chromium.org/12223064/diff/6001/content/common/gpu/gpu_mem... content/common/gpu/gpu_memory_manager.h:167: // On Android only, estimate memory limite from Dalvik heap size. s/limite/limit
Good point. That gets rid of all the Android specific code. I'll try moving the code.
On 2013/02/11 22:41:56, epenner wrote: > Good point. That gets rid of all the Android specific code. I'll try moving the > code. Btw, to warn you ahead of time, we scale that by like 0.7, to account for system memory usage (in CalcAvailableFromGpuTotal). You may want to ignore that on Android.
Thanks Chris. Ptal, I think I removed that bit. I'm going to verify with some logging to be sure the right limit is coming through.
lgtm, with couple nits. https://codereview.chromium.org/12223064/diff/10002/content/common/gpu/gpu_me... File content/common/gpu/gpu_memory_manager.cc (right): https://codereview.chromium.org/12223064/diff/10002/content/common/gpu/gpu_me... content/common/gpu/gpu_memory_manager.cc:192: // We don't need to reduce the total on Android, since Nit: can you put this #if inside CalcAvialbleFromGpuTotal? https://codereview.chromium.org/12223064/diff/10002/ui/gl/gl_context_egl.cc File ui/gl/gl_context_egl.cc (right): https://codereview.chromium.org/12223064/diff/10002/ui/gl/gl_context_egl.cc#n... ui/gl/gl_context_egl.cc:186: bool GLContextEGL::GetTotalGpuMemory(size_t* bytes) { Nit: Should this be #if !defined(OS_ANDROID)?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/8015
Presubmit check for 12223064-8015 failed and returned exit status 1.
INFO:root:Found 8 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/common/gpu
ui/gl
base
Presubmit checks took 1.1s to calculate.
Adding piman and jar for OWNERS lgtm.
lgtm
https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... File base/sys_info_android.cc (right): https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:71: // truncate it to reasonable value to avoid overflows later. It appears that this function does some truncation. Is this truncation specific to the heapsize property? Should it be moved to that function, and not apply to the growth limit? If this data is all "user writable," should we be more concerned with overflows during our calculations?
https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... File base/sys_info_android.cc (right): https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:71: // truncate it to reasonable value to avoid overflows later. The property can only be changed by the root user. So this is really a sanity check for rooted device case. And the "writable" applies to all properties including both heapsize and heapgrowthlimit. On 2013/02/12 17:25:24, jar wrote: > It appears that this function does some truncation. Is this truncation specific > to the heapsize property? Should it be moved to that function, and not apply to > the growth limit? > > If this data is all "user writable," should we be more concerned with overflows > during our calculations?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/8015
Presubmit check for 12223064-8015 failed and returned exit status 1.
INFO:root:Found 8 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ui/gl
base
Presubmit checks took 5.8s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Adding gman and apatrick for ui/gl LGTM. Regarding base, klobag as addressed the concern there. Are we good?
https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... File base/sys_info_android.cc (right): https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:71: // truncate it to reasonable value to avoid overflows later. nit; Please update the comment to indicate that all values are writable, and that you want to sanity check everything. Why is there no concern for overflows? Is there no problem with getting a super-low value "by mistake?" I'm not familiar with the range of these values, but it seems strange to multiply an int64 by 2^30, and then divide it by 2^20. Clearly the code could be more careful... but I'm not familiar with the ranges, and the implication of an overflow-induced error. Please either fix the code to avoid the risk, or add a comment justifying the current code. IMO, you should just fix the code. Example: Create a second variable called "divisor". Make sure that in all cases, either divisor or factor is 1. Then do: result = result * factor / divisor; To be real careful, you could check (when factor is not 1) that there was no overflow. p.s., nit: suggest rename factor to multiplier. On 2013/02/12 18:33:42, klobag.chromium wrote: > The property can only be changed by the root user. So this is really a sanity > check for rooted device case. > > And the "writable" applies to all properties including both heapsize and > heapgrowthlimit. > > On 2013/02/12 17:25:24, jar wrote: > > It appears that this function does some truncation. Is this truncation > specific > > to the heapsize property? Should it be moved to that function, and not apply > to > > the growth limit? > > > > If this data is all "user writable," should we be more concerned with > overflows > > during our calculations? > https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:72: result = std::min<int64>(std::max<int64>(32, result), 1024); Is it really the case that all values should be in the range of 32MB to 1GB, as indicated by this statement? I have no idea what HeapGrowthLimit is.... but this is a surprising restriction to hear based on what I would have guessed for a heap. [Which shows I'm uneducated in this application specific code :-/ ]
Ptal. I'm hoping this addresses the concerns. If there is a larger concern with how the code works or the fact that it sanitizes the value, I'm hoping we can address that in a follow up bug, given the impact that would have on all callers of the function. https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... File base/sys_info_android.cc (right): https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:71: // truncate it to reasonable value to avoid overflows later. I have to admit I didn't look at this parsing logic but rather just reused the function. Given your concern I've looked at it and tried address your concerns. The function now returns bytes only, and the divisor is only applied after the value is sanitized, outside of the parse function. https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... base/sys_info_android.cc:72: result = std::min<int64>(std::max<int64>(32, result), 1024); Grace can say better than me, but yes I think it's a pretty reasonable expectation for this value, and protects us from users who root their device and set these values.
On 2013/02/13 03:33:39, epenner wrote: > Ptal. I'm hoping this addresses the concerns. If there is a larger concern with > how the code works or the fact that it sanitizes the value, I'm hoping we can > address that in a follow up bug, given the impact that would have on all callers > of the function. > > https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... > File base/sys_info_android.cc (right): > > https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... > base/sys_info_android.cc:71: // truncate it to reasonable value to avoid > overflows later. > I have to admit I didn't look at this parsing logic but rather just reused the > function. Given your concern I've looked at it and tried address your concerns. > The function now returns bytes only, and the divisor is only applied after the > value is sanitized, outside of the parse function. > > https://chromiumcodereview.appspot.com/12223064/diff/8015/base/sys_info_andro... > base/sys_info_android.cc:72: result = std::min<int64>(std::max<int64>(32, > result), 1024); > Grace can say better than me, but yes I think it's a pretty reasonable > expectation for this value, and protects us from users who root their device and > set these values. heapgrowthlimit is the maximum Java heap size a normal Java app can use. Even on a 512M device, it is 48m. So 32m is a safe lower bound, at least for android devices.
re: base changes. see comments below. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:66: CHECK('0' <= str[length - 1] && str[length - 1] <= '9'); It is bothersome that the un-tagged number is limited to the range up to 10 billion - 1, but none of the other tagged ranges are checked. This function should be consistent, and have simple semantics (always returns a number betwene 32M and 1024^3?), which would be an int. This really should be written IMO to be consistent. The translation on line 69 should be done first (before line 56). The multiplier should then be applied (probably in each if clause) *after* validating that the translated number is in a range to not overflow when scaled up. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:71: return result * unit_multiplier; This newer code is still problematic, as it ignores all overflows as well. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:81: result = std::min<int64>(std::max<int64>(32 * MB, result), 1024 * MB) / MB; This bounding was partially (almost) done on line 66 in some, but not all cases :-/. Given that it is seemingly a requirement of the parsing, it should all be done up in that translation code as described. Given that the parsing function never (should) return a value larger than 1024^3, it should really just return an int, and you wouldn't need to cast over here on line 82, even before you scaled it down by dividing.
https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:92: result = std::min<int64>(std::max<int64>(16 * MB, result), 1024 * MB) / MB; Why do you clamp this to a different range than the other property (see line 81, and note that one uses 16 as a min, while the other uses 32)? https://codereview.chromium.org/12223064/diff/12004/content/common/gpu/gpu_me... File content/common/gpu/gpu_memory_manager_unittest.cc (left): https://codereview.chromium.org/12223064/diff/12004/content/common/gpu/gpu_me... content/common/gpu/gpu_memory_manager_unittest.cc:531: // We take the lowest GPU's total memory as the limit nit: This comment probably should not be removed. At best, you may have a complex comment incorporating line 523 as well (and talking about android case). https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.cc File ui/gl/gl_context_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.... ui/gl/gl_context_android.cc:61: uint64 heap_size = base::SysInfo::DalvikHeapSizeMB(); Why are we using uint64 here? The range of the function should be 32 to 1024, so there should be no need for such a large value. https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.... ui/gl/gl_context_android.cc:73: *bytes = dalvik_limit; I'm surprised this assignment didn't generate a compiler warning when a uint64 into a size_t on a 32 bit platform. Is that warning disabled? Am I misunderstanding context? I turns out that limit will always be less than or equal to 1024, so davlik_limit will always be less than 2^30, and fit nicely into an int (and we'd avoid this more questionable code). https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_egl.cc File ui/gl/gl_context_egl.cc (right): https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_egl.cc#n... ui/gl/gl_context_egl.cc:185: #if !defined(OS_ANDROID) nit: Given that you didn't ifdef the declaration, why is the implementation ifdef'ed?
I've scoped this down to the parsing changes first, but I've addressed all your comments. Since the other changes aren't in this CL anymore, let me know if I haven't addressed your concerns, or if you want me to add you as reviewer on those changes too. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:66: CHECK('0' <= str[length - 1] && str[length - 1] <= '9'); Sorry, how do you figure it's limited to 10billion - 1 exactly? This CHECK only seems to only validate that right-most digit (left of the tag) is an integer? Or am I missing something? https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:71: return result * unit_multiplier; Thanks, good point! So, I guess I'll need to add overflow protection depending on the k/m/g tag, since each unit could overflow differently? I'll get started on that. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:81: result = std::min<int64>(std::max<int64>(32 * MB, result), 1024 * MB) / MB; Since we will want to re-use the parsing function in the future, I don't think we should limit it to 1GB. Assuming I can fix that function to not overflow an int64, is this code acceptable as is? Each system value might have different expected ranges, which is why I put the clamp in here. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:92: result = std::min<int64>(std::max<int64>(16 * MB, result), 1024 * MB) / MB; The heap growth limit is typically half of the heap size, again, assuming a user hasn't rooted his/her device and changed it. https://codereview.chromium.org/12223064/diff/12004/content/common/gpu/gpu_me... File content/common/gpu/gpu_memory_manager_unittest.cc (left): https://codereview.chromium.org/12223064/diff/12004/content/common/gpu/gpu_me... content/common/gpu/gpu_memory_manager_unittest.cc:531: // We take the lowest GPU's total memory as the limit On 2013/02/14 01:48:41, jar wrote: > nit: This comment probably should not be removed. At best, you may have a > complex comment incorporating line 523 as well (and talking about android case). Done. https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.cc File ui/gl/gl_context_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.... ui/gl/gl_context_android.cc:61: uint64 heap_size = base::SysInfo::DalvikHeapSizeMB(); I think the uint64 crept in from the other code. However the return value is size_t, and size_t is usually a 64 bit unsigned integer on 32/64 bit systems. I think the best thing will just be to use *only* size_t and do static_cast<size_t>(base::SysInfo::DalvikHeapSizeMB()). https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_android.... ui/gl/gl_context_android.cc:73: *bytes = dalvik_limit; See above, I think size_t is uint64 on both 32/64 bit systems, but I'll change everything to size_t given that's what this function expects as output. https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_egl.cc File ui/gl/gl_context_egl.cc (right): https://codereview.chromium.org/12223064/diff/12004/ui/gl/gl_context_egl.cc#n... ui/gl/gl_context_egl.cc:185: #if !defined(OS_ANDROID) The function is defined in the "gl_context_android.cc" file, which is only in the android build. The cleanest way I suppose would be to make an entirely new derived class such that we could override this method on Android only, but given the "_android" file already exists to serve a similar purpose, I was hoping this was acceptable for this minor platform difference.
Ptal. Added overflow protection.
Please add the comments explaining the choices for bounding the values, and consider my other comment below. Then LGTM. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:66: CHECK('0' <= str[length - 1] && str[length - 1] <= '9'); You are correct. I misread the code in a fast (mistaken) glance and mistakenly read that the "string length was less than 9." No good excuse for my mis-read. The code only validates that the last character is a digit. If you look at the definition of StringToInt64, if the last character is not a digit, then the string will (at a minimum) have "trailing characters," and that will cause the conversion function to return false, so line 70 will fail. Bottom line: no need to check this, but I don't really mind it either way. My pref would be to remove the check on line 66, but I'll leave it to you to decide. On 2013/02/14 04:18:36, epenner wrote: > Sorry, how do you figure it's limited to 10billion - 1 exactly? This CHECK only > seems to only validate that right-most digit (left of the tag) is an integer? Or > am I missing something? https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:81: result = std::min<int64>(std::max<int64>(32 * MB, result), 1024 * MB) / MB; OK. On 2013/02/14 04:18:36, epenner wrote: > Since we will want to re-use the parsing function in the future, I don't think > we should limit it to 1GB. Assuming I can fix that function to not overflow an > int64, is this code acceptable as is? Each system value might have different > expected ranges, which is why I put the clamp in here. https://codereview.chromium.org/12223064/diff/12004/base/sys_info_android.cc#... base/sys_info_android.cc:92: result = std::min<int64>(std::max<int64>(16 * MB, result), 1024 * MB) / MB; nit: Please add comments explaining where these bounding values come from, especially to justify different limits in different cases (re: line 92 and line 81). On 2013/02/14 04:18:36, epenner wrote: > The heap growth limit is typically half of the heap size, again, assuming a user > hasn't rooted his/her device and changed it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/22003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/35001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/35001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/12223064/35001
Message was sent while issue was closed.
Change committed as 182986 |
