Backport: [#3446] Optimize ContextN putAllInto

[koo-taejin]

No description provided.

[OlegDokuka]

@koo-taejin can you please rework defaultPutAllOtherIsAbstractContext so it does not fail.

Also, it would be nice to see a test that also demonstrates the reduction in the number of allocated instances of Context

[koo-taejin]

@OlegDokuka Ok. I will check it.

[koo-taejin]

I have updated. :)

[OlegDokuka]

@koo-taejin have noticed a few more things. let's work through it to ensure we are not breaking the original method behavior documented in the CoreContext class. Also, please rollback the test, since it was checking putAllInto literally puts context of Context A into ContextB

[koo-taejin]

@OlegDokuka

It failed.

The reason is that have already made new ConextN object.

Documentation doesn't seem to be a problem(documentation doesn't say that the same class comes out. And CoreContext are already being retyped.) , but it seems best to maintain compatibility. 😭

In order to keep it as similar as possible.
Changing ContextN's PutAllInto to the following looks good:

@Override
public Context putAllInto(Context base) {
    if (base instanceof CoreContext) { // ContextN -> CoreContext 
        ContextN newContext = new ContextN(base.size() + this.size());
        newContext.putAll(base. readOnly()); // newContext.putAll((Map<Object, Object>) base); ->  newContext.putAll(base. readOnly());
        newContext.putAll((Map<Object, Object>) this);
        return newContext;
    }

    Context[] holder = new Context[]{base};
    forEach((k, v) -> holder[0] = holder[0].put(k, v));
    return holder[0];
}

I would wait your advice.

Thanks :)

[koo-taejin]

@OlegDokuka
I've rolled that back to what you said.
As mentioned above, it will work as before for custom Context to maintain compatibility.
However, for CoreContext, it will work more efficiently than before.

[OlegDokuka]

Look good to me! Good job!

One minor comment to resolve and we can merge that

[OlegDokuka]

Can we also use Mockito constructor instrumentation to see that we allocate an object exactly once?

[koo-taejin]

I have checked that the test fails, so I am going to fix the code to make it work. 😭

I don't know if it can be verified with Mockito's mock or spy, because creating a new ContextN object internally.
I am going to check to see if Mockito has support for this.

[koo-taejin]

Mockito's spy() and mock() can't transform the final class, so I used a Proxy.

And I changed the production code like below.

newContext.putAll(base.readOnly()); -> ((CoreContext) base).unsafePutAllInto(newContext);

Thanks :)

[reactorbot]

@OlegDokuka this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

[OlegDokuka]

Good work, @koo-taejin! Look forward to seeing more contributions from you ;)