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.

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

Poor performance of GET /api/node/status #2348

Closed
4miners opened this Issue Aug 29, 2018 · 4 comments

Comments

Projects
3 participants
@4miners
Member

4miners commented Aug 29, 2018

Expected behavior

Performance of API GET /api/node/status should be satisfactory (under 1 second).

Actual behavior

We run some benchmarks with following spec:

CPU: 1vCPU (Digital Ocean) 
RAM: 1 GB Memory
Disk: 25 GB Disk
Connections: 10 
Time: 30s

And got the following results;

Fastest: 3.17
Slowest: 5.34
Average: 4.36

Steps to reproduce

Query endpoints mentioned above.

Which version(s) does this affect? (Environment, OS, etc...)

1.0.1 (after indexes)

@nazarhussain nazarhussain changed the title from Poor performance of API endpoints to Poor performance of /api/node/status Aug 29, 2018

@nazarhussain nazarhussain changed the title from Poor performance of /api/node/status to Poor performance of GET /api/node/status Aug 29, 2018

@nazarhussain

This comment has been minimized.

Show comment
Hide comment
@nazarhussain

nazarhussain Aug 29, 2018

Contributor

We're getting a performance hit here because it executes internally a query which performs COUNT on trs table, which comes with a cost.

modules.transactions.shared.getTransactionsCount((err, count) => {

Contributor

nazarhussain commented Aug 29, 2018

We're getting a performance hit here because it executes internally a query which performs COUNT on trs table, which comes with a cost.

modules.transactions.shared.getTransactionsCount((err, count) => {

@MaciejBaj

This comment has been minimized.

Show comment
Hide comment
@MaciejBaj

MaciejBaj Aug 29, 2018

Member

Not to break API so far the best solution is to cache the confirmed transactions count results after the first hit and invalidate after a new block gets applied or deleted.

Member

MaciejBaj commented Aug 29, 2018

Not to break API so far the best solution is to cache the confirmed transactions count results after the first hit and invalidate after a new block gets applied or deleted.

@4miners

This comment has been minimized.

Show comment
Hide comment
@4miners

4miners Aug 30, 2018

Member

I would like to propose following solution:

  • when calling the endpoint for the first time - store transactions count together with actual block height (modules.blocks.lastBlock.get().height) as an object in the memory
  • when calling the endpoint for the next time - check if block height changed - if so, re-execute the query and update the count together with new block height

Please review, @nazarhussain.

Member

4miners commented Aug 30, 2018

I would like to propose following solution:

  • when calling the endpoint for the first time - store transactions count together with actual block height (modules.blocks.lastBlock.get().height) as an object in the memory
  • when calling the endpoint for the next time - check if block height changed - if so, re-execute the query and update the count together with new block height

Please review, @nazarhussain.

@MaciejBaj MaciejBaj added this to Open Issues in Version 1.0.0 via automation Aug 30, 2018

@MaciejBaj MaciejBaj modified the milestone: Version 1.0.2 Aug 30, 2018

@MaciejBaj MaciejBaj added this to New Issues in Version 1.1.0 via automation Aug 30, 2018

@MaciejBaj MaciejBaj removed this from Open Issues in Version 1.0.0 Aug 30, 2018

@nazarhussain

This comment has been minimized.

Show comment
Hide comment
@nazarhussain

nazarhussain Aug 30, 2018

Contributor

I was just thinking over the idea, in the above solution, we have to recompute the transaction count cache every 10sec, while most of the time its not required due to empty blocks. So we need to think of any other way to actually utilize that cache for longer time.

Contributor

nazarhussain commented Aug 30, 2018

I was just thinking over the idea, in the above solution, we have to recompute the transaction count cache every 10sec, while most of the time its not required due to empty blocks. So we need to think of any other way to actually utilize that cache for longer time.

@MaciejBaj MaciejBaj added this to the Version 1.1.0 milestone Aug 30, 2018

4miners added a commit that referenced this issue Aug 31, 2018

Merge pull request #2359 from LiskHQ/2348-node-status-performance
Poor performance of GET /api/node/status - Closes #2348

Version 1.1.0 automation moved this from New Issues to Closed Issues Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment