Bug 60017 - Performance optimisation : Nullify comment field
Summary: Performance optimisation : Nullify comment field
Status: RESOLVED WONTFIX
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 3.0
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2016-08-18 15:43 UTC by UbikLoadPack support
Modified: 2016-08-20 14:45 UTC (History)
1 user (show)



Attachments
Patch implementing the enhancement (1.46 KB, patch)
2016-08-20 13:30 UTC, UbikLoadPack support
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description UbikLoadPack support 2016-08-18 15:43:37 UTC
Currently comment field are useless during the run of a test plan but very useful to allow readability and maintainability of scripts.

It would be useful to nullify this field so that it does not occupy useless memory during the load test and avoid its transfer on the wire for distributed tests.

As it is possible that some users have used this field for something in their test (which would be a bad practice), we could introduce a property to allow cancelling this behaviour if really needed.

Thoughts ?
Thanks
@ubikloadpack Team
Comment 1 Sebb 2016-08-18 20:54:48 UTC
Is the memory taken by comments really that much?

I don't think it's worth the risk of affecting existing test plans.
Also it will be more work for some users who have to enable/disable the option.

Unless it can be shown to have a significant effect on memory usage, I am -1.
Comment 2 Philippe Mouawad 2016-08-18 21:35:10 UTC
(In reply to Sebb from comment #1)
> Is the memory taken by comments really that much?
> 
> I don't think it's worth the risk of affecting existing test plans.
> Also it will be more work for some users who have to enable/disable the
> option.
> 
> Unless it can be shown to have a significant effect on memory usage, I am -1.

I made a test, you multiply number of thread  * number of element * comment size.
It is possible that the space is highly reduced by using -XX:+UseStringDeduplication which AFAIK is not enable by default. 

Anyway, shouldn't we gain whatever we can in memory footprint ot Test plans ? 

Regarding impacted test plan:
- Either we add a property to revert behaviour (but as you wrote, it's more code to maintain ...)
- My opinion is that why should we take into account hacks and bad practices ? We just add a breaking change in release notes and that's it
Comment 3 Sebb 2016-08-18 21:40:17 UTC
Many test plans will only have a few comments, in which case the change will not help much.

I don't think it's worth the risk.
Comment 4 UbikLoadPack support 2016-08-20 13:30:33 UTC
Created attachment 34166 [details]
Patch implementing the enhancement
Comment 5 Sebb 2016-08-20 14:37:48 UTC
Thanks for the patch, but it not complete.
Also there is not yet agreement that it should be applied.

For the record:

The patch does not address:
- making the behaviour optional
- documentation of the change
- unit tests

Furthermore, the change to AbstractTestElement affects all properties, not just comments, so may cause problems elsewhere. This could be avoided by changing the PreCompiler to drop the comment property entirely instead of setting the content to null. That would save more space.

I am still -1 on applying this change