Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946753003:
Sped up allocateBranchIsland by skipping whole regions of allocated memory. (Closed)
|
Created:
3 years, 6 months ago by Boris Vidolov Modified:
3 years, 6 months ago CC:
chromium-reviews, awong1, brettw Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpeed up allocateBranchIsland by skipping whole regions of allocated memory.
BUG=730918
Review-Url: https://codereview.chromium.org/2946753003
Cr-Commit-Position: refs/heads/master@{#481125}
Committed: https://chromium.googlesource.com/chromium/src/+/9cea65766cfffee8db51e2f6edb694e0fe10ef93
Patch Set 1 #Patch Set 2 : Minor comment updates. #Patch Set 3 : Add a patch file. #
Total comments: 5
Patch Set 4 : Added overflow and underflow detection. #
Total comments: 9
Patch Set 5 : Minor updates to the logic and comments per mmentovai's suggestion. #Patch Set 6 : Clean up the README file. #
Total comments: 1
Patch Set 7 : Fixing release build #
Messages
Total messages: 30 (12 generated)
borisv@chromium.org changed reviewers: + erikchen@chromium.org, mark@chromium.org, rsesek@chromium.org
Colleagues, please take a look at the change. I would especially appreciate looking into the vm_region usage and the instruction opcode manipulations. Also, please advise if I should go ahead and update the code to use an absolute 64-bit address instead of keeping around the 32-bit jump out of the overridden function.
In the CL description, write in the present tense (“Speed up” instead of “sped up”). https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... third_party/mach_override/mach_override.c:415: vm_address_t first = (uint64_t)originalFunctionAddress - kMaxJumpOffset; 1. The actual range for a jump back should be 0x80000000, but kMaxJumpOffset is 0x7fffffff. But I don’t really care. 2. It’s possible to end up with first > last, which is bad. All it would take is for originalFunctionAddress to be in the range of 31 bits. https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... third_party/mach_override/mach_override.c:430: #if defined(__x86_64__) This doesn’t need to be x86_64-only, but I’d understand that you might not want to touch the ppc stuff. Maybe you can make this be x86_64/i386, though.
Description was changed from ========== Sped up allocateBranchIsland by skipping whole regions of allocated memory. BUG=730918 ========== to ========== Speed up allocateBranchIsland by skipping whole regions of allocated memory. BUG=730918 ==========
Thanks, Mark. See my answers and my latest patch. https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... third_party/mach_override/mach_override.c:415: vm_address_t first = (uint64_t)originalFunctionAddress - kMaxJumpOffset; On 2017/06/20 15:46:42, Mark Mentovai wrote: > 1. The actual range for a jump back should be 0x80000000, but kMaxJumpOffset is > 0x7fffffff. But I don’t really care. > > 2. It’s possible to end up with first > last, which is bad. All it would take is > for originalFunctionAddress to be in the range of 31 bits. #1 Yep, I know :). #2 Not sure if this is possible - it will only happen if the 63-rd bit is set and I believe that that bit is used by the address system. I added underflow and overflow detection, though. See my latest patch. https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... third_party/mach_override/mach_override.c:430: #if defined(__x86_64__) On 2017/06/20 15:46:42, Mark Mentovai wrote: > This doesn’t need to be x86_64-only, but I’d understand that you might not want > to touch the ppc stuff. Maybe you can make this be x86_64/i386, though. The conditional compilation is hard to follow (if you want, I will add comments after each #endif), but the i386 code seem to do the right thing at line #402 above. It allocates a random page with VM_ANYWHERE. Within the 32-bit address space this should be sufficient, I believe - the JMP can't go beyond the 32-bit delta. Now, there is a an issue of locality - we probably want pages to be relatively close together. Yet, I would not optimize the code for that unless there is a specific and measured performance concern.
https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/40001/third_party/mach_overri... third_party/mach_override/mach_override.c:415: vm_address_t first = (uint64_t)originalFunctionAddress - kMaxJumpOffset; Boris Vidolov wrote: > On 2017/06/20 15:46:42, Mark Mentovai wrote: > > 1. The actual range for a jump back should be 0x80000000, but kMaxJumpOffset > is > > 0x7fffffff. But I don’t really care. > > > > 2. It’s possible to end up with first > last, which is bad. All it would take > is > > for originalFunctionAddress to be in the range of 31 bits. > > #1 Yep, I know :). > #2 Not sure if this is possible - it will only happen if the 63-rd bit is set > and I believe that that bit is used by the address system. I added underflow and > overflow detection, though. See my latest patch. Oh, I was thinking 32-bit addresses when I read this earlier. OK…
LGTM. https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/README.chromium (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/README.chromium:3: Version: 0 ? Can you put this back how it was? https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:272: // E9 (jmp 32-bit relative to RIP). Then we should update the You don’t need “the” at the end of the line. https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:439: kern_return_t region_result = You can just reuse err here. And you can reuse page in place of region_start.
LGTM https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:271: // TODO: On 64-bit, move to opcode FF/4 (jmp 64-bit absolute) instead of I looked at this and I think the main concern would be it encodes as more bytes and may require knowing how to eat/save more instructions of the original function than mach_override currently knows how to do. But obviously not dealing with that now :).
https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:271: // TODO: On 64-bit, move to opcode FF/4 (jmp 64-bit absolute) instead of Robert Sesek wrote: > I looked at this and I think the main concern would be it encodes as more bytes > and may require knowing how to eat/save more instructions of the original > function than mach_override currently knows how to do. But obviously not dealing > with that now :). Oh, also, it’s not 64-bit absolute as the comment says, but indirect absolute. It’d take an instruction sequence to achieve…
Thanks, Mark and Robert. I updated the code per suggestion, but I could not restore the README.chromium, as the tools enforce a specific version now (or 0, if the revision is tracked). https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/README.chromium (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/README.chromium:3: Version: 0 On 2017/06/20 20:32:33, Mark Mentovai wrote: > ? > > Can you put this back how it was? 'git cl upload' errored out on me, requiring a version. I had to hack it to work. Per README template, this should be 0, if the version is tracked by the revision. To satisfy the tools, I have set it to 0 and kept the comment on the next line. https://cs.chromium.org/chromium/src/third_party/README.chromium.template https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:271: // TODO: On 64-bit, move to opcode FF/4 (jmp 64-bit absolute) instead of On 2017/06/20 21:08:53, Mark Mentovai wrote: > Robert Sesek wrote: > > I looked at this and I think the main concern would be it encodes as more > bytes > > and may require knowing how to eat/save more instructions of the original > > function than mach_override currently knows how to do. But obviously not > dealing > > with that now :). > > Oh, also, it’s not 64-bit absolute as the comment says, but indirect absolute. > It’d take an instruction sequence to achieve… @rsesek: Indeed, overriding more bytes is what I am scared of. The code uses fixed number of bytes (5) below and possibly in other places (e.g. the reentry island code). @mark: Good point - I updated the comment. https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:272: // E9 (jmp 32-bit relative to RIP). Then we should update the On 2017/06/20 20:32:34, Mark Mentovai wrote: > You don’t need “the” at the end of the line. Done. https://codereview.chromium.org/2946753003/diff/60001/third_party/mach_overri... third_party/mach_override/mach_override.c:439: kern_return_t region_result = On 2017/06/20 20:32:34, Mark Mentovai wrote: > You can just reuse err here. And you can reuse page in place of region_start. Done. I needed to step through the code a lot, hence the extra variables left and right...
The CQ bit was checked by borisv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2946753003/#ps80001 (title: "Minor updates to the logic and comments per mmentovai's suggestion.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/21 00:02:21, Boris Vidolov wrote: > Thanks, Mark and Robert. I updated the code per suggestion, but I could not > restore the README.chromium, as the tools enforce a specific version now (or 0, > if the revision is tracked). OK, then just remove the Version line, and the text that followed it. Don’t leave a weirdo unstructured “newer than” line in the structured key-value area.
The CQ bit was unchecked by borisv@chromium.org
On 2017/06/21 00:04:36, Mark Mentovai wrote: > On 2017/06/21 00:02:21, Boris Vidolov wrote: > > Thanks, Mark and Robert. I updated the code per suggestion, but I could not > > restore the README.chromium, as the tools enforce a specific version now (or > 0, > > if the revision is tracked). > > OK, then just remove the Version line, and the text that followed it. Don’t > leave a weirdo unstructured “newer than” line in the structured key-value area. Ok, weirdo line removed, but Version is required. Per tool recommendation, changed it to "unknown".
The CQ bit was checked by borisv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2946753003/#ps100001 (title: "Clean up the README file.")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2946753003/diff/100001/third_party/mach_overr... File third_party/mach_override/mach_override.c (right): https://codereview.chromium.org/2946753003/diff/100001/third_party/mach_overr... third_party/mach_override/mach_override.c:277: assert(addressOffset64 == addressOffset); Ah, yes, asserts compile to nothing in release builds. You can wrap line 275 in #ifndef NDEBUG unless you want to cook up something else, or you can get rid of addressOffset64 as an independent varaible and intsead write its right-hand-side expression right here, with an int64_t cast.
On 2017/06/21 03:38:24, Mark Mentovai wrote: > https://codereview.chromium.org/2946753003/diff/100001/third_party/mach_overr... > File third_party/mach_override/mach_override.c (right): > > https://codereview.chromium.org/2946753003/diff/100001/third_party/mach_overr... > third_party/mach_override/mach_override.c:277: assert(addressOffset64 == > addressOffset); > Ah, yes, asserts compile to nothing in release builds. You can wrap line 275 in > #ifndef NDEBUG unless you want to cook up something else, or you can get rid of > addressOffset64 as an independent varaible and intsead write its right-hand-side > expression right here, with an int64_t cast. Yep, I should have seen it coming :). These days I am too used to Keystone's KS_ASSERT compiling in release builds too.
The CQ bit was checked by borisv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2946753003/#ps120001 (title: "Fixing release build")
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": 120001, "attempt_start_ts": 1498025734169680,
"parent_rev": "a8a293697a89e19a3ac294cf303f18d7e41237d8", "commit_rev":
"9cea65766cfffee8db51e2f6edb694e0fe10ef93"}
Message was sent while issue was closed.
Description was changed from ========== Speed up allocateBranchIsland by skipping whole regions of allocated memory. BUG=730918 ========== to ========== Speed up allocateBranchIsland by skipping whole regions of allocated memory. BUG=730918 Review-Url: https://codereview.chromium.org/2946753003 Cr-Commit-Position: refs/heads/master@{#481125} Committed: https://chromium.googlesource.com/chromium/src/+/9cea65766cfffee8db51e2f6edb6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9cea65766cfffee8db51e2f6edb6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
