Skip to content

optim: mark a few classes final #10809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Aug 21, 2017
Merged

optim: mark a few classes final #10809

laanwj merged 2 commits into from
Aug 21, 2017

Conversation

@theuni
Copy link
Member

@theuni theuni commented Jul 12, 2017

Using gcc's -Wsuggest-final-types and lto, I identified a few easy devirtualization wins:

wallet/wallet.h:651:7: warning: Declaring type 'struct CWallet' final would enable devirtualization of 26 calls [-Wsuggest-final-types]

coins.h:201:7: warning: Declaring type 'struct CCoinsViewCache' final would enable devirtualization of 13 calls [-Wsuggest-final-types]

txdb.h:67:7: warning: Declaring type 'struct CCoinsViewDB' final would enable devirtualization of 5 calls [-Wsuggest-final-types]

zmq/zmqnotificationinterface.h:16:7: warning: Declaring type 'struct CZMQNotificationInterface' final would enable devirtualization of 4 calls [-Wsuggest-final-types]

httpserver.cpp:42:7: warning: Declaring type 'struct HTTPWorkItem' final would enable devirtualization of 2 calls [-Wsuggest-final-types]

@meshcollider
Copy link
Contributor

meshcollider commented Jul 12, 2017

test/coins_tests.cpp:74:7: error: cannot derive from ‘final’ base ‘CCoinsViewCache’ in derived type ‘{anonymous}::CCoinsViewCacheTest’ class CCoinsViewCacheTest : public CCoinsViewCache

@sipa
Copy link
Member

sipa commented Jul 12, 2017

test/coins_tests.cpp:74:7: error: cannot derive from ‘final’ base ‘CCoinsViewCache’ in derived type ‘{anonymous}::CCoinsViewCacheTest’ class CCoinsViewCacheTest : public CCoinsViewCache

You can fix that by renaming the existing CCoinsViewCache to a non-final CCoinsViewCacheBase, and have CCoinsViewCache become a final subclass of that with no implementation except a constructor. Then CCoinsViewCacheTest can become another subclass of CCoinsViewCacheBase.

@meshcollider meshcollider mentioned this pull request Jul 13, 2017
Closed
@sipa sipa mentioned this pull request Jul 16, 2017
Closed
@practicalswift
Copy link
Contributor

practicalswift commented Jul 17, 2017

@theuni Two more devirtualization opportunities (from closed PR #10847):

  • Declaring CCoinsViewErrorCatcher final enables devirtualization of two calls
  • Declaring CReserveKey final enables devirtualization of one call

@theuni
Copy link
Member Author

theuni commented Jul 17, 2017

@practicalswift I'll pull in the other two, but I think we should drop the CCoinsViewCache one. The inheritance there is hard enough to follow already.

@practicalswift
Copy link
Contributor

practicalswift commented Jul 17, 2017

@theuni Sounds great! :-)

theuni and others added 2 commits July 17, 2017 11:34
@theuni
optim: mark a few classes final
9a1675e
@practicalswift @theuni
Enable devirtualization opportunities by using the final specifier (C…
40a0f9f 
…++11)

* Declaring CCoinsViewErrorCatcher final enables devirtualization of two calls
* Declaring CReserveKey final enables devirtualization of one call
gmaxwell
gmaxwell approved these changes Jul 18, 2017
View reviewed changes
Copy link
Contributor

@gmaxwell gmaxwell left a comment

Choose a reason for hiding this comment

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

utACK

@practicalswift
Copy link
Contributor

practicalswift commented Jul 19, 2017

utACK 40a0f9f

1 similar comment
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jul 19, 2017

utACK 40a0f9f

@paveljanik
Copy link
Contributor

paveljanik commented Aug 10, 2017

utACK 40a0f9f

@laanwj laanwj merged commit 40a0f9f into bitcoin:master Aug 21, 2017
laanwj added a commit that referenced this pull request Aug 21, 2017
@laanwj
Merge #10809: optim: mark a few classes final
c559884 
40a0f9f Enable devirtualization opportunities by using the final specifier (C++11) (practicalswift)
9a1675e optim: mark a few classes final (Cory Fields)

Pull request description:

  Using gcc's ```-Wsuggest-final-types``` and lto, I identified a few easy devirtualization wins:

  > wallet/wallet.h:651:7: warning: Declaring type 'struct CWallet' final would enable devirtualization of 26 calls [-Wsuggest-final-types]

  >coins.h:201:7: warning: Declaring type 'struct CCoinsViewCache' final would enable devirtualization of 13 calls [-Wsuggest-final-types]

  >txdb.h:67:7: warning: Declaring type 'struct CCoinsViewDB' final would enable devirtualization of 5 calls [-Wsuggest-final-types]

  >zmq/zmqnotificationinterface.h:16:7: warning: Declaring type 'struct CZMQNotificationInterface' final would enable devirtualization of 4 calls [-Wsuggest-final-types]

  >httpserver.cpp:42:7: warning: Declaring type 'struct HTTPWorkItem' final would enable devirtualization of 2 calls [-Wsuggest-final-types]

Tree-SHA512: 2a825fd27121ccabaacff5cde2fc8a50d1b4cc846374606caa2a71b0cd8fcb0d3c9b5b3fd342d944998610e2168048601278f8a3709cc515191a0bb2d98ba782
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 9, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#10809: optim: mark a few classes final
b0a3d23 
40a0f9f Enable devirtualization opportunities by using the final specifier (C++11) (practicalswift)
9a1675e optim: mark a few classes final (Cory Fields)

Pull request description:

  Using gcc's ```-Wsuggest-final-types``` and lto, I identified a few easy devirtualization wins:

  > wallet/wallet.h:651:7: warning: Declaring type 'struct CWallet' final would enable devirtualization of 26 calls [-Wsuggest-final-types]

  >coins.h:201:7: warning: Declaring type 'struct CCoinsViewCache' final would enable devirtualization of 13 calls [-Wsuggest-final-types]

  >txdb.h:67:7: warning: Declaring type 'struct CCoinsViewDB' final would enable devirtualization of 5 calls [-Wsuggest-final-types]

  >zmq/zmqnotificationinterface.h:16:7: warning: Declaring type 'struct CZMQNotificationInterface' final would enable devirtualization of 4 calls [-Wsuggest-final-types]

  >httpserver.cpp:42:7: warning: Declaring type 'struct HTTPWorkItem' final would enable devirtualization of 2 calls [-Wsuggest-final-types]

Tree-SHA512: 2a825fd27121ccabaacff5cde2fc8a50d1b4cc846374606caa2a71b0cd8fcb0d3c9b5b3fd342d944998610e2168048601278f8a3709cc515191a0bb2d98ba782
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 3, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10809: optim: mark a few classes final
62b2f3a 
40a0f9f Enable devirtualization opportunities by using the final specifier (C++11) (practicalswift)
9a1675e optim: mark a few classes final (Cory Fields)

Pull request description:

  Using gcc's ```-Wsuggest-final-types``` and lto, I identified a few easy devirtualization wins:

  > wallet/wallet.h:651:7: warning: Declaring type 'struct CWallet' final would enable devirtualization of 26 calls [-Wsuggest-final-types]

  >coins.h:201:7: warning: Declaring type 'struct CCoinsViewCache' final would enable devirtualization of 13 calls [-Wsuggest-final-types]

  >txdb.h:67:7: warning: Declaring type 'struct CCoinsViewDB' final would enable devirtualization of 5 calls [-Wsuggest-final-types]

  >zmq/zmqnotificationinterface.h:16:7: warning: Declaring type 'struct CZMQNotificationInterface' final would enable devirtualization of 4 calls [-Wsuggest-final-types]

  >httpserver.cpp:42:7: warning: Declaring type 'struct HTTPWorkItem' final would enable devirtualization of 2 calls [-Wsuggest-final-types]

Tree-SHA512: 2a825fd27121ccabaacff5cde2fc8a50d1b4cc846374606caa2a71b0cd8fcb0d3c9b5b3fd342d944998610e2168048601278f8a3709cc515191a0bb2d98ba782
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@kallewoof kallewoof kallewoof approved these changes

@dcousens dcousens dcousens approved these changes

@gmaxwell gmaxwell gmaxwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.