Skip to main content

Please review fix for CRs 6836117 and 6824104

5 replies [Last post]
sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

Hi all,
please review my fix for next related bugs:

CR 6836117:
"TemplateParameterFilter should not unnecessary produce new InterviewParameters objects"

and

CR
"provide method to InterviewParameters#dispose() to clean up the interview objects"

https://jtharness.dev.java.net/source/browse/jtharness?rev=1528&view=rev

Some comments:
1) InterviewParameters.dispose() method should be overwritten by client code, if they store links on their interview objects. This will prevent from memory leaks when harness creates new InterviewParameters instances for temporal needs. Harness, by turn, should invoke this method each time InterviewParameters instances becomes unnecessary.

2) Parameter filter doesn't need to store link on Parameters object, so TemplateParameterFilter may dispose template params after filter updated with data from template.

Thanks,
Sergey

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
bkurotsu
Offline
Joined: 2004-12-13
Points: 0

Thanks. Integrated without changes. Trunk rev 1567.

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

So I assume the changes in ConfigHandler are mostly to remove dead code and unused parameters?

ContextManager - It is commented, what does that mean?

ExecTool - How much thought did you put into the order in which things are disposed?

ParameterFilter - code was commented out when updating, does the still work? Looked suspect. I'l try to test it.

Otherwise looks fine. I'm happy to integrate everything except ParameterFilter.

sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

ConfigHandler - yes

ContextManager - just a comment, why we shouldn't invoke it here, I may delete

ExecTool - while I didn't found any problems here, it may be more safe to invoke it after reportHandler.dispose()

ParameterFilter - sorry, didn't understood your question. The problem was that ParameterFilter hold link to InterviewParameters object, that used in update() method. The reason, as I may see, was in performance - if links were different, there was no need in CompositeFilter.equals(newFilters, filters) invokation. But memory leak seems more important here, so I simplified that code.

fda
Offline
Joined: 2005-05-27
Points: 0

I agree with Sergei, the ParameterFilter update doesn't change the functionality, it affects only performance but very little. I don't see any objections against integration Sergei's fix.

fda
Offline
Joined: 2005-05-27
Points: 0

Fix looks good. I found one more class com.sun.javatest.EditJTI, which creates instances of InterviewParameters.

One minor note: Javadoc comments to the dispose() method is silent that method does nothing by default and designed to be overridden when cleaning is desired.