reduce map contention

[yanns]

• re-use hash from cacheKeyReversed when possible
• compute the values atomically

[avelx]

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

[yanns]

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

Yes, one would need to add some benchmarks. In the scala world, we usually use https://github.com/sbt/sbt-jmh

[avelx]

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

Yes, one would need to add some benchmarks. In the scala world, we usually use https://github.com/sbt/sbt-jmh

I guess

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

Yes, one would need to add some benchmarks. In the scala world, we usually use https://github.com/sbt/sbt-jmh

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

Yes, one would need to add some benchmarks. In the scala world, we usually use https://github.com/sbt/sbt-jmh

sangria.util.Cache is using

yanns - I would assume this should be possible to test this one way or another, or use some kind of benchmarking like: jmeter or something ?

Yes, one would need to add some benchmarks. In the scala world, we usually use https://github.com/sbt/sbt-jmh

sangria.util.Cache is just a wrapper around ConcurrentHashMap, so not sure if we really need to write tests / benchmarks around this? I would think this is just a draft PR in this case ?

[yanns]

sangria.util.Cache is just a wrapper around ConcurrentHashMap, so not sure if we really need to write tests / benchmarks around this? I would think this is just a draft PR in this case ?

No a benchmark should not check this, but the general impact of the change, with a resolution using synchronous and asynchronous code.

[avelx]

Reference in ne

I understand your intention,
The only difference I see is that we are replacing custom-made getElseOrUpdate with
build in "computeIfAbsent"

There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

[yanns]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"

There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

[avelx]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"
There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

We need to look for tests around Executor, as I can see there is an "ExecutorSpec" UT,
Do we have Integration Tests (ITs) against sangria-core ? looks like this is something worth to set up to have some level of assurance that these changes works as expected, I will dig around existing UTs, but I don't think this is by nature can cover test part for this change

[yanns]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"
There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

We need to look for tests around Executor, as I can see there is an "ExecutorSpec" UT, Do we have Integration Tests (ITs) against sangria-core ? looks like this is something worth to set up to have some level of assurance that these changes works as expected, I will dig around existing UTs, but I don't think this is by nature can cover test part for this change

One of the change in this PR is how concurrent load of the same key are handled. Before, each load is processed, and each saves a value into the map. With this PR, only one call is doing the loading, saving the value into the map. The other load is waiting for the first one to process.
I'm not sure that tests in ExecutorSpec really check how concurrency.

And we don't have any ITs. When I want to asses a new change more deeply, I publish the library locally, and use it in several internal web services. But this process does not scale, and is bind to the fact that I have access to several internal applications using sangria that offers a lot of tests.

[avelx]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"
There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

We need to look for tests around Executor, as I can see there is an "ExecutorSpec" UT, Do we have Integration Tests (ITs) against sangria-core ? looks like this is something worth to set up to have some level of assurance that these changes works as expected, I will dig around existing UTs, but I don't think this is by nature can cover test part for this change

One of the change in this PR is how concurrent load of the same key are handled. Before, each load is processed, and each saves a value into the map. With this PR, only one call is doing the loading, saving the value into the map. The other load is waiting for the first one to process. I'm not sure that tests in ExecutorSpec really check how concurrency.

And we don't have any ITs. When I want to asses a new change more deeply, I publish the library locally, and use it in several internal web services. But this process does not scale, and is bind to the fact that I have access to several internal applications using sangria that offers a lot of tests.

I think we can just raise an Github issue and park this conversation as there are quite a list of challanges here like to add ITs to the coRe project etc. Let me know if you happy for me to do this.

[yanns]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"
There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

We need to look for tests around Executor, as I can see there is an "ExecutorSpec" UT, Do we have Integration Tests (ITs) against sangria-core ? looks like this is something worth to set up to have some level of assurance that these changes works as expected, I will dig around existing UTs, but I don't think this is by nature can cover test part for this change

One of the change in this PR is how concurrent load of the same key are handled. Before, each load is processed, and each saves a value into the map. With this PR, only one call is doing the loading, saving the value into the map. The other load is waiting for the first one to process. I'm not sure that tests in ExecutorSpec really check how concurrency.
And we don't have any ITs. When I want to asses a new change more deeply, I publish the library locally, and use it in several internal web services. But this process does not scale, and is bind to the fact that I have access to several internal applications using sangria that offers a lot of tests.

I think we can just raise an Github issue and park this conversation as there are quite a list of challanges here like to add ITs to the coRe project etc. Let me know if you happy for me to do this.

Thanks for proposing. Any help appreciated here!

[avelx]

Reference in ne

I understand your intention, The only difference I see is that we are replacing custom-made getElseOrUpdate with build in "computeIfAbsent"
There must be existing tests we can use to check if there is no impact, or we need to add a new one to assess impact and see how Executor behaves after

To be honest, I don't think that we have enough test coverage about a potential impact. That's reason why I haven't continued with this change as I was not feeling confident enough.

We need to look for tests around Executor, as I can see there is an "ExecutorSpec" UT, Do we have Integration Tests (ITs) against sangria-core ? looks like this is something worth to set up to have some level of assurance that these changes works as expected, I will dig around existing UTs, but I don't think this is by nature can cover test part for this change

One of the change in this PR is how concurrent load of the same key are handled. Before, each load is processed, and each saves a value into the map. With this PR, only one call is doing the loading, saving the value into the map. The other load is waiting for the first one to process. I'm not sure that tests in ExecutorSpec really check how concurrency.
And we don't have any ITs. When I want to asses a new change more deeply, I publish the library locally, and use it in several internal web services. But this process does not scale, and is bind to the fact that I have access to several internal applications using sangria that offers a lot of tests.

I think we can just raise an Github issue and park this conversation as there are quite a list of challanges here like to add ITs to the coRe project etc. Let me know if you happy for me to do this.

Thanks for proposing. Any help appreciated here!

Here is an issue for this work: #1284

Feel free to comment on the issue itself, I think we can close this thread.