NoThreadClone should be used more often

[Vladimir Sitnikov <si...@gmail.com>]

Hi,

I've noticed that we have just a few cases when NoThreadClone is used.
It results in enormous memory consumption since the test plan is
effectively duplicated for each and every thread.

Just in case: my testcase is 1 thread group (100'000 threads), 10
transaction controllers (each has one FlowControlAction, and one Constant
30sec timer).
The test does not fit within 2Gb RAM :-(


AFAIK there are three types of elements:
* (almost) Stateless. For instance, UniformRandomTimer has no fields, and
it does not even access its own properties.
Note: there's `getRandom()` which is now implemented as
`ThreadLocalRandom.current()` so it is thread-safe
* Semi-stateless. For instance, TestAction. It does have a state for
cancellation purposes (it needs to remember which thread to cancel)
* Elements that do hold state. For example, TransactionController has quite
a few fields

I think we should do something with that, otherwise, the memory consumption
is insane.

I see the following possibilities:
a) Implement copy-on-write like mode for properties.
For instance, we can add `modifiedPropMap` for the properties that were
modified in the running version.
Then we could share the same property map and newly modified properties (if
any) would go another map.
That would save a lot of memory by small changes to the core alone.

Just some numbers:
400'000 TestActions retain 315MiB in the heap, and the most part of it
(300MiB) is consumed by property maps

b) Apply NoThreadClone when possible. Note: `NoThreadClone` interface
propagates for the derived classes,
however, it might be the case that the derived class does have state, thus
it requires cloning.
So I think we might need to add `@ThreadClone` annotation which would
enable us to specify different cloning behaviors for parent and derived
classes.

Then we might want to add a test case that ensures all in-core components
either support NoThreadClone or explicitly declare that they do not support
it.

c) Consider dropping some attributes when making a runtime version.
For instance, TestAction has the following properties:
ActionProcessor.action
ActionProcessor.duration
ActionProcessor.target
TestElement.enabled
TestElement.gui_class
TestElement.name
TestElement.test_class
TestElement.comments

I guess gui_class and test_class are not really required for the running
version.
It might play well with annotation:
`@ThreadClone(excludeProperties=["...gui_class", "test_class"])`

Any thoughts?

Vladimir

Re: NoThreadClone should be used more often

[Antonio Gomes Rodrigues <ra...@gmail.com>]

Thanks to your answer


Le lun. 30 mars 2020 à 14:56, Vladimir Sitnikov <si...@gmail.com>
a écrit :

> >https://github.com/JCTools/JCTools) instead of Dexx HashMap?
>
> They serve for completely different use cases.
>
> JCTool's map is for concurrent operation (like Java's ConecurrentHashMap).
> That is when a map needs to be accessed from multiple threads, and each
> thread should see updates that come from the other threads.
>
> However, JMeter needs quite the opposite.
> JMeter needs a map that pretends to be **non-shared**.
> I other words, the map should support updates, however, each thread should
> see **only** its own updates.
>
> What JMeter currently does it **clones** maps. So a single
> "FlowControlAction" is stored millions times in the Java heap
> since JMeter clones the test plan for each and every thread.
>
> What we need is a way to share "the common values", however, all the
> updates must be put to separate storage.
> This is exactly what Dexx collections do.
>
> Of course, Dexx is not the only implementation, however it was the best for
> performance-memory footprint ratio when I explored the libraries some time
> ago for https://github.com/vlsi/compactmap
>
> Vladimir
>

Re: NoThreadClone should be used more often

[Vladimir Sitnikov <si...@gmail.com>]

>https://github.com/JCTools/JCTools) instead of Dexx HashMap?

They serve for completely different use cases.

JCTool's map is for concurrent operation (like Java's ConecurrentHashMap).
That is when a map needs to be accessed from multiple threads, and each
thread should see updates that come from the other threads.

However, JMeter needs quite the opposite.
JMeter needs a map that pretends to be **non-shared**.
I other words, the map should support updates, however, each thread should
see **only** its own updates.

What JMeter currently does it **clones** maps. So a single
"FlowControlAction" is stored millions times in the Java heap
since JMeter clones the test plan for each and every thread.

What we need is a way to share "the common values", however, all the
updates must be put to separate storage.
This is exactly what Dexx collections do.

Of course, Dexx is not the only implementation, however it was the best for
performance-memory footprint ratio when I explored the libraries some time
ago for https://github.com/vlsi/compactmap

Vladimir

Re: NoThreadClone should be used more often

[Antonio Gomes Rodrigues <ra...@gmail.com>]

+1 to put it in 6.0

Vladimir, one question : Why not use JCTools (
https://github.com/JCTools/JCTools) instead of Dexx HashMap?



Le lun. 30 mars 2020 à 14:05, Vladimir Sitnikov <si...@gmail.com>
a écrit :

> >What do you mean by unsafe  ?
>
> Suppose the property in question is TestElement.gui_class
> If the code reads it as <<String
> getPropertyAsString("TestElement.gui_class")>>, then the property can't be
> modified,
> so it is safe to use a shared property instance.
>
> However, if the client code does <<JMeterProperty prop =
> getProperty("TestElement.gui_class");>> then
> we have no idea what will happen next. The code might call
> <<prop.setObjectValue(...)>> or do something else.
>
> That is why it is not safe to share the properties that were exposed
> outside of AbstractTestElement class.
>
> >For me this should be 6.0 (even if it's backward compatible),
> >it's a major change that needs to be highlighted.
>
> That is true. Async executor would indeed qualify as 6.0.
>
> Vladimir
>

Re: NoThreadClone should be used more often

[Vladimir Sitnikov <si...@gmail.com>]

>What do you mean by unsafe  ?

Suppose the property in question is TestElement.gui_class
If the code reads it as <<String
getPropertyAsString("TestElement.gui_class")>>, then the property can't be
modified,
so it is safe to use a shared property instance.

However, if the client code does <<JMeterProperty prop =
getProperty("TestElement.gui_class");>> then
we have no idea what will happen next. The code might call
<<prop.setObjectValue(...)>> or do something else.

That is why it is not safe to share the properties that were exposed
outside of AbstractTestElement class.

>For me this should be 6.0 (even if it's backward compatible),
>it's a major change that needs to be highlighted.

That is true. Async executor would indeed qualify as 6.0.

Vladimir

Re: NoThreadClone should be used more often

[Philippe Mouawad <ph...@gmail.com>]

On Mon, Mar 30, 2020 at 1:34 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:

> >Custom scripts can also modify props (I guess you have this in mind, but
> >just not to skip it)
>
> That is true. I expect that all calls to "public JMeterProperty
> getProperty(String key)" should be treated as unsafe.
>
What do you mean by unsafe  ?

>
> Frankly speaking, it would probably be great if we could split
> configuration and execution phases.
> Currently, JMeter uses the same object (e.g. AbstractSampler) to configure
> the behavior in the test plan, and the very same object is used for the
> execution.
>
> However, the goals there are completely different:
> 1) Configuration needs backward compatibility (e.g. load old test plans),
> configuration needs various setters, generic visitor for "search substring"
> features, and so on
> 2) Execution needs much less. It does not need UI, it needs no backward
> compatibility. It just needs enough information to execute stuff.
>
> It would be much better if we
> removed org.apache.jmeter.testelement.AbstractTestElement#setRunningVersion
> and replaced it with something like
> `AbstractTestElement#getExecutorNode()`
>



>
> Then we would be able to build the executable test plan just once and skip
> cloning the test plan nodes altogether.
> Unfortunately, it is not clear how much effort would it take to rewrite the
> existing samplers and the controllers to the new approach :(
>
> So the plan could probably be like:
> JMeter 5.3 or 5.4:
> rewrite org.apache.jmeter.testelement.AbstractTestElement#propMap to use
> Dexx HashMap;  add coroutine-based executor. That would unlock testing with
> high number of threads.
>

For me this should be 6.0 (even if it's backward compatible), it's a major
change  that needs to be highlighted.

JMeter 6.0: split AbstractTestElement into "configuration" and "execution"
> classes
>
> Vladimir
>


-- 
Cordialement.
Philippe Mouawad.

Re: NoThreadClone should be used more often

[Vladimir Sitnikov <si...@gmail.com>]

>Custom scripts can also modify props (I guess you have this in mind, but
>just not to skip it)

That is true. I expect that all calls to "public JMeterProperty
getProperty(String key)" should be treated as unsafe.

Frankly speaking, it would probably be great if we could split
configuration and execution phases.
Currently, JMeter uses the same object (e.g. AbstractSampler) to configure
the behavior in the test plan, and the very same object is used for the
execution.

However, the goals there are completely different:
1) Configuration needs backward compatibility (e.g. load old test plans),
configuration needs various setters, generic visitor for "search substring"
features, and so on
2) Execution needs much less. It does not need UI, it needs no backward
compatibility. It just needs enough information to execute stuff.

It would be much better if we
removed org.apache.jmeter.testelement.AbstractTestElement#setRunningVersion
and replaced it with something like
`AbstractTestElement#getExecutorNode()`

Then we would be able to build the executable test plan just once and skip
cloning the test plan nodes altogether.
Unfortunately, it is not clear how much effort would it take to rewrite the
existing samplers and the controllers to the new approach :(

So the plan could probably be like:
JMeter 5.3 or 5.4:
rewrite org.apache.jmeter.testelement.AbstractTestElement#propMap to use
Dexx HashMap;  add coroutine-based executor. That would unlock testing with
high number of threads.
JMeter 6.0: split AbstractTestElement into "configuration" and "execution"
classes

Vladimir

Re: NoThreadClone should be used more often

[Philippe Mouawad <ph...@gmail.com>]

On Mon, Mar 30, 2020 at 1:06 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:

> >a) Implement copy-on-write like mode for properties.
> >For instance, we can add `modifiedPropMap` for the properties that were
> modified in the running version.
>
> copy-on-write seems trivial to implement (e.g. replace Map with Dexx
> HashMap: https://github.com/andrewoma/dexx ),
> however, there's a major problem:
> JMeterProperty is exposed in a raw fashion (e.g. #getProperty or even
> #propertyIterator)
>
> It means we should clone properties at the time they are exposed to the
> untrusted code.
> However, it is not clear how to tell if the property was already cloned or
> not.
>

Custom scripts can also modify props (I guess you have this in mind, but
just not to skip it)

>
> Vladimir
>


-- 
Cordialement.
Philippe Mouawad.

Re: NoThreadClone should be used more often

[Vladimir Sitnikov <si...@gmail.com>]

>a) Implement copy-on-write like mode for properties.
>For instance, we can add `modifiedPropMap` for the properties that were
modified in the running version.

copy-on-write seems trivial to implement (e.g. replace Map with Dexx
HashMap: https://github.com/andrewoma/dexx ),
however, there's a major problem:
JMeterProperty is exposed in a raw fashion (e.g. #getProperty or even
#propertyIterator)

It means we should clone properties at the time they are exposed to the
untrusted code.
However, it is not clear how to tell if the property was already cloned or
not.

Vladimir