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

Add DatabaseClient bind variant for list of positioned parameters #33274

Closed
wanyongx opened this issue Jul 25, 2024 · 7 comments
Closed

Add DatabaseClient bind variant for list of positioned parameters #33274

wanyongx opened this issue Jul 25, 2024 · 7 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Milestone

Comments

@wanyongx
Copy link

wanyongx commented Jul 25, 2024

https://github.com/spring-projects/spring-framework/blob/65faca8236124e0ae37ff68afc2366f7ccf286d2/spring-r2dbc/src/main/java/org/springframework/r2dbc/core/DefaultDatabaseClient.java#L264C2-L274C4

these bind methods new too many times when many binds.

@Override
public DefaultGenericExecuteSpec bind(int index, Object value) {
	assertNotPreparedOperation();
	Assert.notNull(value, () -> String.format(
			"Value at index %d must not be null. Use bindNull(…) instead.", index));

	Map<Integer, Parameter> byIndex = new LinkedHashMap<>(this.byIndex);
	byIndex.put(index, resolveParameter(value));

	return new DefaultGenericExecuteSpec(byIndex, this.byName, this.sqlSupplier, this.filterFunction);
}

A batch interface can be provided or some optimize so that the following two objects can be created only once

new LinkedHashMap<>   
new DefaultGenericExecuteSpec

I do a test to check the performance:

        @Test
	public void test() {
		DefaultGenericExecuteSpec spec = new DefaultGenericExecuteSpec(() -> "select 1");

		long start = System.currentTimeMillis();
		for (int i = 0; i < 10000; i++){
			// use the default method
			spec = spec.bind(i, "string");
		}
		System.out.println("default method cost: " + (System.currentTimeMillis() - start));

		start = System.currentTimeMillis();
		for (int i = 0; i < 10000; i++){
			// use the optimized method
			spec = spec.bind2(i, "string");
		}
		System.out.println("optimized method cost: " + (System.currentTimeMillis() - start));
	}


        public DefaultGenericExecuteSpec bind2(int index, Object value) {
			assertNotPreparedOperation();
			Assert.notNull(value, () -> String.format(
					"Value at index %d must not be null. Use bindNull(…) instead.", index));
			if(byIndex.isEmpty()) {
				Map<Integer, Parameter> byIndex = new LinkedHashMap<>(this.byIndex);
				byIndex.put(index, resolveParameter(value));

				return new DefaultGenericExecuteSpec(byIndex, this.byName, this.sqlSupplier, this.filterFunction);
			}

			byIndex.put(index, resolveParameter(value));
			return this;
		}

the result is

default method cost: 1790
optimized method cost: 5

more details, my project need insert large amount of data , about 3000 bind parameters per second. The cpu is high. I analyze to here according to the flame diagram.
image

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 25, 2024
@bclozel
Copy link
Member

bclozel commented Jul 25, 2024

Sorry but this is not enough for us to go on.

How did you measure that this was a performance bottleneck? Do you have profiling data to back this up?
We would rather spend time working on performance enhancements for meaningful optimizations rather than simple hunches.

I'm closing this issue for now, we can reopen if we get actual data.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply in: data Issues in data modules (jdbc, orm, oxm, tx) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 25, 2024
@wanyongx
Copy link
Author

Sorry but this is not enough for us to go on.

How did you measure that this was a performance bottleneck? Do you have profiling data to back this up? We would rather spend time working on performance enhancements for meaningful optimizations rather than simple hunches.

I'm closing this issue for now, we can reopen if we get actual data.

Hello @bclozel , thanks for your reply. I've added some actual data.Could you please check the issue again?

@bclozel bclozel added status: waiting-for-triage An issue we've not yet triaged or decided on and removed status: declined A suggestion or change that we don't feel we should currently apply labels Jul 27, 2024
@bclozel bclozel self-assigned this Jul 27, 2024
@bclozel bclozel reopened this Jul 27, 2024
@bclozel
Copy link
Member

bclozel commented Jul 29, 2024

I have looked at this and I think you are misusing the API.

The GenericExecuteSpec is meant to be stateless and each call should always return a new instance, because developers can reuse the spec instance for several different queries. As a result, we cannot apply the change you are suggesting because this would change the stateless nature of the spec API and this would be a regression.

If you are indeed calling bind many times in a row, you should instead use bindValues(Map<String, ?> source), which should only cause a single allocation for all binding parameters. I'm closing this issue as a result.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 29, 2024
@wanyongx
Copy link
Author

I have looked at this and I think you are misusing the API.

The GenericExecuteSpec is meant to be stateless and each call should always return a new instance, because developers can reuse the spec instance for several different queries. As a result, we cannot apply the change you are suggesting because this would change the stateless nature of the spec API and this would be a regression.

If you are indeed calling bind many times in a row, you should instead use bindValues(Map<String, ?> source), which should only cause a single allocation for all binding parameters. I'm closing this issue as a result.

@bclozel, I know this api bindValues(Map<String, ?> source), but needed is such as bindValues(Map<Integer, ?> source).

@bclozel bclozel changed the title The bind method performance is poor Add DatabaseClient bind variant for list of positioned parameters Jul 29, 2024
@bclozel bclozel added type: enhancement A general enhancement and removed status: declined A suggestion or change that we don't feel we should currently apply labels Jul 29, 2024
@bclozel bclozel removed their assignment Jul 29, 2024
@bclozel bclozel added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 29, 2024
@bclozel
Copy link
Member

bclozel commented Jul 29, 2024

In this case, we should consider this as a variant of #27282.
There are existing equivalents with org.springframework.jdbc.core.simple.JdbcClient.StatementSpec#params(java.util.List<?>) and org.springframework.jdbc.core.simple.JdbcClient.StatementSpec#params(java.lang.Object...) and I think we should go with one of those.

Your bindValues(Map<String, ?> source) suggestion would somehow infer that calling this method multiple times would only partially override the ordered parameters, which I think could be easily confusing if we mix this with org.springframework.r2dbc.core.DatabaseClient.GenericExecuteSpec#bind(int, java.lang.Object) calls.

@bclozel bclozel reopened this Jul 29, 2024
@bclozel bclozel self-assigned this Jul 29, 2024
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 29, 2024
@bclozel bclozel added this to the 6.2.0-M7 milestone Jul 29, 2024
@bclozel
Copy link
Member

bclozel commented Jul 29, 2024

@wanyongx a new bindValues(List<?>) variant will be available in the upcoming 6.2.0-M7, to be released in a couple of weeks.

@wanyongx
Copy link
Author

@wanyongx a new bindValues(List<?>) variant will be available in the upcoming 6.2.0-M7, to be released in a couple of weeks.

@bclozel That's good, bindValues(List<?>) is what I needed.I'll wait for this release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants