|
|
Created:
4 years, 4 months ago by tibell Modified:
4 years, 4 months ago Reviewers:
yzshen1 CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@before-sharing Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove generated Mojo C++ error reporting code out of line
Also switch from scoped_refptr to unique_ptr, as we only have one owner.
Savings:
SafeJsonParserStub::AcceptWithResponder: 42 bytes
SafeJsonParser_Parse_ForwardToCallback: 44 bytes
We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages.
BUG=597125
Committed: https://crrev.com/029b671b558428d07b0054e83451253e3a5db225
Cr-Commit-Position: refs/heads/master@{#408901}
Patch Set 1 #Patch Set 2 : Add copyright statement #Patch Set 3 : Don't factor out ProxyToResponder #Patch Set 4 : Fix compilation #Patch Set 5 : unique_ptr needs to be a friend class #Patch Set 6 : Use public destructor instead of friend class #
Total comments: 6
Patch Set 7 : Address review comments #
Messages
Total messages: 41 (31 generated)
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Fix compilation
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
unique_ptr needs to be a friend class
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Generate less code for Mojo C++ bindings BUG=597125 ========== to ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. BUG=597125 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Use public destructor instead of friend class
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. BUG=597125 ========== to ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 ==========
tibell@chromium.org changed reviewers: + yzshen@chromium.org
I'm still reading through the assembly for the generated code so hopefully I'll find more substantial saving elsewhere. Right now the main cause of bloat seems to be the use of Callbacks. Question: from what I can tell we only need unique_ptr to own the proxy, but I don't know the original reasoning for scoped_refptr so please correct me if I'm wrong. Also note that I fixed a bug in the error messages (they didn't get correctly expanded before).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few nits. Thanks! https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/validation_errors.h (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/validation_errors.h:84: const base::StringPiece& description = ""); nit: does it make sense to use the same type for |description| as ReportValidationError()? https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:100: {%- set desc = class_name~"::"~method.name~" response" %} :D good catch. One thing may be worth considering is to measure how much space such diagnostic string literals take and see whether we need to make improvements there. https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:318: |indent(4)}} nit: now this line could be merged with the one above.
Address review comments
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/validation_errors.h (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/validation_errors.h:84: const base::StringPiece& description = ""); On 2016/07/31 07:16:32, yzshen1 wrote: > nit: does it make sense to use the same type for |description| as > ReportValidationError()? Done. https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:100: {%- set desc = class_name~"::"~method.name~" response" %} On 2016/07/31 07:16:32, yzshen1 wrote: > :D good catch. > > One thing may be worth considering is to measure how much space such diagnostic > string literals take and see whether we need to make improvements there. Agreed. It's on my list. https://codereview.chromium.org/2188143002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:318: |indent(4)}} On 2016/07/31 07:16:32, yzshen1 wrote: > nit: now this line could be merged with the one above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tibell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2188143002/#ps120001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 ========== to ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 ========== to ========== Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 Committed: https://crrev.com/029b671b558428d07b0054e83451253e3a5db225 Cr-Commit-Position: refs/heads/master@{#408901} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/029b671b558428d07b0054e83451253e3a5db225 Cr-Commit-Position: refs/heads/master@{#408901} |