Bug 186123

Summary: emitMove should only emit the move if it's actually needed
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, ggaren, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Patch none

Keith Miller
Reported 2018-05-30 19:06:30 PDT
emitMove should only emit the move if it's actually needed
Attachments
Patch (35.08 KB, patch)
2018-05-30 19:10 PDT, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (79.23 KB, patch)
2018-05-30 20:02 PDT, Keith Miller 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews102 for mac-sierra (2.32 MB, application/zip)
2018-05-30 21:06 PDT, EWS Watchlist 		no flags
Details
Patch (79.23 KB, patch)
2018-05-31 17:10 PDT, Keith Miller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Keith Miller
Comment 1 2018-05-30 19:10:08 PDT
Created attachment 341637 [details] Patch
Geoffrey Garen
Comment 2 2018-05-30 19:48:57 PDT
Comment on attachment 341637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341637&action=review > Source/JavaScriptCore/ChangeLog:10 > + This patch relpaces emitMove with moveToDestinationIfNeeded. This > + will prevent us from emiting moves to the same location. The old > + emitMove, has been renamed to emitMov and made private. That's a lot of semantic weight for the letter "e" to carry. :P How about calling the private function "emitMoveWithoutDestinationCheck". Or you can call the public function "move" and the private function "emitMove". "emit*" in the BytecodeGenerator usually means the lowest level function that splats out an opcode.
Keith Miller
Comment 3 2018-05-30 20:01:58 PDT
Comment on attachment 341637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341637&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + emitMove, has been renamed to emitMov and made private. > > That's a lot of semantic weight for the letter "e" to carry. :P > > How about calling the private function "emitMoveWithoutDestinationCheck". > > Or you can call the public function "move" and the private function "emitMove". "emit*" in the BytecodeGenerator usually means the lowest level function that splats out an opcode. "e" is the important (common) letter in the English language! I'll do the latter.
Keith Miller
Comment 4 2018-05-30 20:02:05 PDT
Created attachment 341641 [details] Patch
EWS Watchlist
Comment 5 2018-05-30 21:06:56 PDT
Comment on attachment 341641 [details] Patch Attachment 341641 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7885426 New failing tests: imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
EWS Watchlist
Comment 6 2018-05-30 21:06:57 PDT
Created attachment 341642 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Geoffrey Garen
Comment 7 2018-05-30 21:13:42 PDT
Is that Mac test failure real?
Keith Miller
Comment 8 2018-05-31 10:54:08 PDT
(In reply to Geoffrey Garen from comment #7) > Is that Mac test failure real? Seems pretty unlikely, although I suppose it could be. IMO, it's probably due to a more fundamental line tracking bug if it is so we should just rebaseline the test and file another bug.
Keith Miller
Comment 9 2018-05-31 17:10:01 PDT
Created attachment 341711 [details] Patch
WebKit Commit Bot
Comment 10 2018-06-01 11:11:42 PDT
Comment on attachment 341711 [details] Patch Clearing flags on attachment: 341711 Committed r232399: <https://trac.webkit.org/changeset/232399>
WebKit Commit Bot
Comment 11 2018-06-01 11:11:43 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2018-06-01 11:16:53 PDT
<rdar://problem/40727627>
Note You need to log in before you can comment on or make changes to this bug.