Skip to content

Add GPR_CODEGEN_DEBUG_ASSERT#17106

Merged
yashykt merged 2 commits intogrpc:masterfrom
yashykt:debugassert
Nov 6, 2018
Merged

Add GPR_CODEGEN_DEBUG_ASSERT#17106
yashykt merged 2 commits intogrpc:masterfrom
yashykt:debugassert

Conversation

@yashykt
Copy link
Copy Markdown
Member

@yashykt yashykt commented Nov 5, 2018

Add GPR_CODEGEN_DEBUG_ASSERT

@yashykt yashykt added release notes: no Indicates if PR should not be in release notes lang/c++ labels Nov 5, 2018
vjpai
vjpai reviewed Nov 5, 2018
View reviewed changes
include/grpcpp/impl/codegen/core_codegen_interface.h Outdated
#ifndef GRPCPP_IMPL_CODEGEN_CORE_CODEGEN_INTERFACE_H
#define GRPCPP_IMPL_CODEGEN_CORE_CODEGEN_INTERFACE_H

#include <cassert>
Copy link
Copy Markdown
Contributor

@vjpai vjpai Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this inclusion needed now when it wasn't used before?

Copy link
Copy Markdown
Member Author

@yashykt yashykt Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NDEBUG needs it right?

Copy link
Copy Markdown
Member Author

@yashykt yashykt Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or I guess we could depend on the users of this to include it instead. That's what Core does right now

Copy link
Copy Markdown
Contributor

@nicolasnoble nicolasnoble Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<cassert> is for the assert() macro. NDEBUG is just a flag that's supposed to be globally defined by the command-line build, such as -DNDEBUG. The default is that neither _DEBUG nor NDEBUG are defined. Build systems such as cmake or bazel will define one of them depending on building in debug or release mode.

Copy link
Copy Markdown
Member Author

@yashykt yashykt Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ofcourse! you are right. That was my moment of stupidity :D
Removed it

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,616      Total (=)      2,020,616

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,939      Total (<)     11,180,943

 No significant differences in binary sizes

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,616      Total (=)      2,020,616

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,947      Total (>)     11,180,941

 No significant differences in binary sizes

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 5, 2018 

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@yashykt
Copy link
Copy Markdown
Member Author

yashykt commented Nov 6, 2018

Known issues : #16212
Thanks for reviewing!

@yashykt yashykt merged commit c6b375b into grpc:master Nov 6, 2018
@vjpai
Copy link
Copy Markdown
Contributor

vjpai commented Nov 7, 2018

This doesn't do the right thing so I'm going to revert. We often do assert(thing_with_side_effects().ok() and this won't do that thing.

@vjpai vjpai mentioned this pull request Nov 7, 2018
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2019
@yashykt yashykt deleted the debugassert branch May 18, 2023 20:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@nicolasnoble nicolasnoble nicolasnoble left review comments

@vjpai vjpai vjpai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lang/c++ release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yashykt @grpc-testing @vjpai @nicolasnoble