Skip to content

Minor optimizations #1206

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
merged 6 commits into from
Oct 18, 2018
Merged

Minor optimizations #1206

merged 6 commits into from
Oct 18, 2018

Conversation

denravonska
Copy link
Member

Small optimizations to slightly improve the sync speeds. This imports 18.5k of testnet blocks from file around 1.5% faster. However, the first ~2k of those are scrypt blocks where the gain is proportionally smaller. If run on a larger block set the gain increase should be higher.

  • Reduce the number of times block hash calculations are done in CheckBlock
  • Avoid converting the CPID to its textual representation in GetLifetimeCPID

Original:

07/09/18 06:09:57 Loaded 18510 blocks from external file in 91614ms
07/09/18 06:11:49 Loaded 18510 blocks from external file in 91227ms

Opt:

07/09/18 06:07:32 Loaded 18510 blocks from external file in 90246ms
07/09/18 06:14:42 Loaded 18510 blocks from external file in 90663ms

@denravonska
Copy link
Member Author

This PR changes the behavior. Now str("") is equal to uint128() which was not the case before.

@TheCharlatan
Copy link
Contributor

Why wasn't this the case before?

@denravonska
Copy link
Member Author

Looks like Im incorrect, but the behavior is still changed. With my changes it's not possible to use the CPID "INVESTOR" which done in a few places. I will have another look but might have to close the PR.

@TheCharlatan
Copy link
Contributor

What is the difference between having the field empty and having "Investor"?

@denravonska
Copy link
Member Author

I am not entirely sure to be honest. Let me do a full sync to see what's being passed to the function.

@denravonska
Copy link
Member Author

Oh, it can never not be an invalid (!= 32 characters) CPID due to this check, but the CPID returned from GetCPID() could either be INVESTOR or empty before whereas now it will be 0x0 for those two instances.

@denravonska denravonska added this to the Camilla milestone Jul 11, 2018
@denravonska
Copy link
Member Author

I did a second run of this with a larger data set:

Original
org Loaded 784965 blocks from external file in 8801140ms
org Loaded 784965 blocks from external file in 8779245ms

Optimized
opt Loaded 784965 blocks from external file in 8429411ms

In this case the sync was ~4.1% faster with the optimizations. It's just a reduction of 6 minutes from 2.4 hours though.

iFoggz
iFoggz approved these changes Jul 22, 2018
View reviewed changes
Copy link
Member

@iFoggz iFoggz left a comment

Choose a reason for hiding this comment

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

Approved for testing. Any amount of inprovements is the right step in the direction. The improvements will pay off more in long term

tomasbrod
tomasbrod suggested changes Jul 29, 2018
View reviewed changes
src/main.cpp Outdated
@@ -5728,7 +5730,7 @@ StructCPID GetLifetimeCPID(const std::string& cpid, const std::string& sCalledFr
CBlockIndex* pblockindex = mapItem->second;
if(pblockindex == NULL ||
pblockindex->IsInMainChain() == false ||
pblockindex->GetCPID() != cpid)
pblockindex->cpid != cpid128)
Copy link
Member

Choose a reason for hiding this comment

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

This will provide different (undefined?) results for non-researcher blocks. Please check the block flags before working with cpid field, to ensure its validity.
Note: cpid128 is fine, because of check at L5707, but accessing cpid of the blockindex is invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are absolutely right, that is the correct way of avoiding issues when the CPID actually is 000000.... I'll add a check for it, thanks.

@denravonska denravonska force-pushed the minor-optimizations branch from 363fa28 to 2a0d0ac Compare July 30, 2018 09:05
@tomasbrod
Copy link
Member

plz do not rebase after reviews, it makes hard to see changes

@denravonska
Copy link
Member Author

The changeset is the same as before the rebase.

@denravonska denravonska mentioned this pull request Aug 7, 2018
Closed
@denravonska
Copy link
Member Author

denravonska commented Aug 10, 2018 •
edited
Loading

Completely removed the genesis block hash check in CheckBlock. This works on import, will try sync.

@denravonska denravonska merged commit 40f40bc into gridcoin-community:development Oct 18, 2018
denravonska added a commit that referenced this pull request Apr 3, 2019
@denravonska
4.0.2.0-leisure
004c6b8 
Added:
 - Add `rainbymagnitude` RPC command #1235 (@Foggyx420).
 - Add stake splitting and side staking #1265 (@jamescowens).
 - Detect and block Windows shutdown so wallet can exit cleanly #1309
   (@jamescowens).
 - Add message support to sendfrom and sendtoaddress #1400 (@denravonska).

Changed:
 - Configuration options are now case insensitive #294 (@Foggyx420).
 - Update command in beaconstatus help message #1312 (@chrstphrchvz).
 - Improve synchronization speeds:
   - Refactor superblock pack/unpack #1194 (@denravonska).
   - Optimize neuralsecurity calculations #1255 (@denravonska).
   - Reduce hash calculations when checking blocks #1206 (@denravonska).
 - Make display of private key in beaconstatus OPT-IN only #1275 (@Foggyx420).
 - Store Beacon keys in Wallet #1088 (@tomasbrod).
 - Use default colors for pie chart #1333 (@chrstphrchvz).
 - Show hand cursor when hovering clickable labels #1332 (@chrstphrchvz).
 - Update README.md #1337 (@Peppernrino).
 - Fix integer overflow with displayed nonce #1297 (@personthingman2).
 - Improve application cache performance #1317 (@denravonska).
 - Improve reorg speeds #1263 (@denravonska).
 - Update Polish translation #1375 (@michalkania).

Fixed:
 - Remove expired polls from overview page #1250 (@personthingman2).
 - Fix plural text on block age #1304 (@scribblemaniac).
 - Fix researcher staking issue if your chain head was staked by you,
   #1299 (@denravonska).
 - Fix incorrect address to grcpool node #1314 (@wilkart).
 - Do not replace underscores by spaces in Qt Poll URLs #1327 (@tomasbrod).
 - Fix scraper SSL issues #1330 (@Foggyx420).

Removed:
 - Remove or merged several RPC commands #1228 (@Foggyx420):
    - `newburnaddress`, removed.
    - `burn2`: Removed.
    - `cpid`: Merged into `projects`.
    - `mymagnitude`: Merged into `magnitude`.
    - `rsa`: Removed, use `magnitude`.
    - `rsaweight`: Removed, use `magnitude`.
    - `proveownership`: Removed.
    - `encrypt`: Removed.
 - Remove obsolete POW fields from RPC responses #1358 (@jamescowens).
 - Remove obsolete netsoft fields for slight RAM requirement reduction
   #1336 (@denravonska).
 - Remove unused attachment functionality #1345 (@denravonska).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@tomasbrod tomasbrod tomasbrod requested changes

@TheCharlatan TheCharlatan TheCharlatan approved these changes

@iFoggz iFoggz iFoggz approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
Camilla
Development

Successfully merging this pull request may close these issues.
4 participants
@denravonska @TheCharlatan @tomasbrod @iFoggz