Skip to content
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

Massive performance improvement... #363

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

typhoonsimon
Copy link

@typhoonsimon typhoonsimon commented Mar 14, 2020

.. for index page and address view page. Don't forget that these additional mongo indexes must be created to achieve the level of performance of http://lcp.altcoinwarz.com/

db.txes.createIndex({total: 1})
db.txes.createIndex({total: -1})
db.txes.createIndex({blockindex: 1})
db.txes.createIndex({blockindex: -1})

addresstxes
db.addresstxes.createIndex({a_id: 1, blockindex: -1})```

... over large chains. Added indexes and brought back last_txs setting (set to 0 will display the whole chain in the index, but without sacrificing server or mongo performance).
@uaktags uaktags linked an issue Mar 14, 2020 that may be closed by this pull request
@typhoonsimon
Copy link
Author

Ah sorry, that was a debug, I will fix it.

@typhoonsimon
Copy link
Author

OK I have put back the correct port settings, sorry about that.

@typhoonsimon
Copy link
Author

it should be noted that there is a diff between count and countDocuments, and by virtue of mongos documentation this should be avoided.

Do you want me to set it back to countDocuments ?

@uaktags
Copy link
Collaborator

uaktags commented Mar 14, 2020

it should be noted that there is a diff between count and countDocuments, and by virtue of mongos documentation this should be avoided.

Do you want me to set it back to countDocuments ?

I'm not entirely sure yet. I need to go through and double check its usage. My fear is improper shutdowns will cause the count to be off, but this is simply the total count of txes for an address. It's not something I deem mission critical compared to say the actual values of transactions/amounts/etc.

Performance wise, count is indeed much faster, but it's also susceptible to inaccuracies.

@typhoonsimon
Copy link
Author

it should be noted that there is a diff between count and countDocuments, and by virtue of mongos documentation this should be avoided.

Do you want me to set it back to countDocuments ?

I'm not entirely sure yet. I need to go through and double check its usage. My fear is improper shutdowns will cause the count to be off, but this is simply the total count of txes for an address. It's not something I deem mission critical compared to say the actual values of transactions/amounts/etc.

Performance wise, count is indeed much faster, but it's also susceptible to inaccuracies.

That's exactly what I thought. It's the same as count without lock on MySQL InnoDB, the count is not accurate but is very fast. In this case, worst that can happen is the the page number display is not accurate. But transactions per address are slow, I guess we could live with that given the cost of the countDocuments().

@uaktags
Copy link
Collaborator

uaktags commented Mar 15, 2020

it should be noted that there is a diff between count and countDocuments, and by virtue of mongos documentation this should be avoided.

Do you want me to set it back to countDocuments ?

I'm not entirely sure yet. I need to go through and double check its usage. My fear is improper shutdowns will cause the count to be off, but this is simply the total count of txes for an address. It's not something I deem mission critical compared to say the actual values of transactions/amounts/etc.
Performance wise, count is indeed much faster, but it's also susceptible to inaccuracies.

That's exactly what I thought. It's the same as count without lock on MySQL InnoDB, the count is not accurate but is very fast. In this case, worst that can happen is the the page number display is not accurate. But transactions per address are slow, I guess we could live with that given the cost of the countDocuments().

I agree. I would however counter to provide a settings config so that explorer admins can choose to have accuracy. The performance impact from if/then statements would be negligible.

@allavett
Copy link

allavett commented Jun 1, 2020

Any update on the ETA of this pull request being finalized and merged?

Also, @typhoonsimon where does the need come to index both descending and ascending directions, does it really give the desired result?

.. Don't forget that these additional mongo indexes must be created to achieve the level of performance of http://lcp.altcoinwarz.com/

db.txes.createIndex({total: 1})
db.txes.createIndex({total: -1})
db.txes.createIndex({blockindex: 1})
db.txes.createIndex({blockindex: -1})

@uaktags
Copy link
Collaborator

uaktags commented Jul 9, 2020

I'm going to add this to my testing and merges with uaktags:explorer#58. Once testing is complete, I'll push accept and merge this. Hopefully end of week for an eta

uaktags added a commit to uaktags/explorer that referenced this pull request Jul 10, 2020
Copy link
Collaborator

@TheHolyRoger TheHolyRoger left a comment

Choose a reason for hiding this comment

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

As per my comments this needs adjusting and/or duplicating into latest TX and latest BLOCK function calls

@@ -27,7 +27,7 @@ html
function update_stats(){
$.ajax({url: '/ext/summary', success: function(json){
$("#supply").text(parseInt(parseFloat(json.data[0].supply).toFixed(0)).toLocaleString('en'));
$("#difficulty").text(parseFloat(json.data[0].difficulty).toFixed(2));
$("#difficulty").text(json.data[0].difficulty);
Copy link
Collaborator

@TheHolyRoger TheHolyRoger Jul 16, 2020

Choose a reason for hiding this comment

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

Why are we changing this? If you want this formatted differently then we should add config options for it in a separate PR
Edit: sorry my fault I broke this for hybrid diff

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in b7180dc

});
}, function() {
exit();

// insert all at once after creation
Copy link
Collaborator

@TheHolyRoger TheHolyRoger Jul 16, 2020

Choose a reason for hiding this comment

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

this looks good, much better way of doing it

return cb(txs, count);
}
});
lib.get_blockcount(function(blockcount) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@typhoonsimon As per the function's name "get_last_txs_ajax" - this function does NOT show latest blocks - it shows Latest Transactions, we cannot use the blockcount call here.

This does look very efficient though and would work almost as is for an additional function for latest blocks, which we could then set in the config for the user to display latest TX or latest BLOCKS on the front page.

In terms of making this code work for latest TXs what's the performance difference of Tx.countDocuments vs Tx.count here?

tx_count might be a stat we should keep updated via block indexing?

@TheHolyRoger
Copy link
Collaborator

TheHolyRoger commented Jul 16, 2020

Merged the index declarations to 'main' here: 3f81b75 - these alone seem to have improved paging performance for me so thanks for that @typhoonsimon

@TheHolyRoger
Copy link
Collaborator

d611f8f This patch seems to add a mild speed improvement until we resolve this PR but thanks for your help so far @typhoonsimon

@TheHolyRoger
Copy link
Collaborator

Difficulty display fixed: b7180dc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants