feature(db): optimize properties query frequency by CarlChaoCarl · Pull Request #5378

feature(db): optimize properties query frequency by CarlChaoCarl · Pull Request #5378 · tronprotocol/java-tron

chaozhu added 3 commits

May 30, 2023 12:05

…into feature/optimize_properties_query_frequency_v6

		isOptimized = snapshot.isOptimized();
		if (isOptimized) {
		if (root == previous) {
		Streams.stream(root.iterator()).forEach( e -> put(e.getKey(),e.getValue()));

Choose a reason for hiding this comment

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

reformat the code style, put the white space to the right place

Choose a reason for hiding this comment

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

These two if statements can be combined

Choose a reason for hiding this comment

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

👌🏻

Choose a reason for hiding this comment

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

Why reserve these if they are useless?

Choose a reason for hiding this comment

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

I removed this part of the commented code

		SnapshotImpl fromImpl = (SnapshotImpl) from;
		Streams.stream(fromImpl.db).forEach(e ->
		{
		if (db.get(e.getKey()) == null && e.getValue() != null ) {

Choose a reason for hiding this comment

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

code format

Choose a reason for hiding this comment

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

👌🏻

		Streams.stream(fromImpl.db).forEach(e -> db.put(e.getKey(), e.getValue()));
		}

		public void mergeFullData(Snapshot from) {

Choose a reason for hiding this comment

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

As I see, the merge method is used for merging the snapshotImpl which is behind itself, this method mergeFullData is used for merging the snapshotImpl which is ahead, and only one snapshot. it only can merge all data by another outside iteration.

So renaming the method name to mergeAhead or mergeBefore or mergeFront may be better?

Choose a reason for hiding this comment

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

OK, I renamed the mergeFullData function to mergeAhead


		--activeSession;

		dbs.forEach(db -> {

Choose a reason for hiding this comment

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

Why set the reload logic in commit(), commit() only invoked by the end of pushing a block, not a transaction.

Are there other reasons for doing this, can you explain?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

This is because if the transaction ends without a commit,
Revoke is needed at this time,
session.close is called,
Then call snapshotManager.revoke,
Then there is no need to perform reload logic.
So the reload logic is only executed when the transaction commits.

@tomatoishealthy @halibobo1205

Any issue associated with this PR? So we can have a comprehensive background

halibobo1205 changed the title ~~feature(mechanism): optimize properties query frequency~~ feature(db): optimize properties query frequency

Aug 3, 2023

Any issue associated with this PR? So we can have a comprehensive background

This Issue is associated with the current pr
#5375

chaozhu added 2 commits

August 9, 2023 17:17

…_properties_query_frequency_v6

		}
		SnapshotImpl fromImpl = (SnapshotImpl) from;
		Streams.stream(fromImpl.db).forEach(e -> {
		if (db.get(e.getKey()) == null && e.getValue() != null) {

Choose a reason for hiding this comment

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

How would happen for e.getValue() == null?

Choose a reason for hiding this comment

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

It is necessary to count which keys are null and whether the access frequency is high

Choose a reason for hiding this comment

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

db.get(e.getKey()) == null && e.getValue() != null
Passing these two judgments means:

The current db does not have this key, that is, db.get(e.getKey()) is null,
At the same time, the snapshotimpl from has this key and the value is not empty, that is, e.getValue() != null.

@wubin01 @halibobo1205

Choose a reason for hiding this comment

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

How would happen for e.getValue() == null?

@halibobo1205 is it possible that key exists but value is null in the property database?

Choose a reason for hiding this comment

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

@yuekun0707 , It shouldn't be null.

		SnapshotImpl fromImpl = (SnapshotImpl) from;
		Streams.stream(fromImpl.db).forEach(e ->
		{
		if (db.get(e.getKey()) == null && e.getValue() != null ) {

Choose a reason for hiding this comment

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

Will there be a frequently accessed key with a value of null？

Choose a reason for hiding this comment

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

db.get(e.getKey()) == null && e.getValue() != null
Passing these two judgments means:

The current db does not have this key, that is, db.get(e.getKey()) is null,
At the same time, the snapshotimpl from has this key and the value is not empty, that is, e.getValue() != null.

@wubin01

		isOptimized = snapshot.isOptimized();
		if (isOptimized) {
		if (root == previous) {
		Streams.stream(root.iterator()).forEach( e -> put(e.getKey(),e.getValue()));

Choose a reason for hiding this comment

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

These two if statements can be combined


		dbs.forEach(db -> {
		if (db.getHead().isOptimized()) {
		db.getHead().reloadToMem();

Choose a reason for hiding this comment

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

isOptimized judged twice

Choose a reason for hiding this comment

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

OK,I get it

Choose a reason for hiding this comment

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

I have fixed this

		}
		SnapshotImpl fromImpl = (SnapshotImpl) from;
		Streams.stream(fromImpl.db).forEach(e -> {
		if (db.get(e.getKey()) == null && e.getValue() != null) {

Choose a reason for hiding this comment

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

It is necessary to count which keys are null and whether the access frequency is high

Better to add the unit test for these changes

chaozhu and others added 2 commits

August 30, 2023 15:18

chaozhu and others added 4 commits

August 31, 2023 20:25

Better to add the unit test for these changes

@tomatoishealthy Yeah, I have added unit tests.