Project

General

Profile

Feature #7365

Avoid using reflection in generating BDT

Added by Alexandru Lungu about 3 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Radu Apetrii
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
production:
No
env_name:
topics:

History

#1 Updated by Alexandru Lungu about 3 years ago

  • Tracker changed from Bug to Feature

TextOps.instantiate is a hot-spot in our profiling. This is because it uses getDeclaredConstructor().newInstance(), which does dynamic invocation.

Replace this with a BaseDataTypeFactory which does a faster native look-up (if - else if - else kind of job). Do this refactoring to TextOps.instantiateUnknown, BDT.generateUnknown, BDT.generateDefault, BDT.calcFormatLength and other places with getDeclaredConstructor().newInstance() that resolve to a BDT instance.

#2 Updated by Alexandru Lungu about 3 years ago

  • Status changed from New to WIP
  • Assignee set to Radu Apetrii

#3 Updated by Alexandru Lungu about 3 years ago

Radu, please mark this your top priority. The bottle-neck is visible here, so we may want to see it finished asap. This way I will have some time to review, profile and test before the end of this week. Thank you!

#4 Updated by Radu Apetrii about 3 years ago

As suggested, I created BaseDataTypeFactory and I replaced all the occurrences that resolve to a BaseDataType instance (I hope so).
The commit is on 7365a, rev.14578. I have also tested with a large customer application and found no problems.

I have one (small) concern though. There is a change in FieldReference.unknownValue which might result in a conflict when merged with the changes from #7329. I believe this is for another time, but if there will be problems, I can provide feedback.

#5 Updated by Alexandru Lungu about 3 years ago

  • % Done changed from 0 to 80

Radu. I know we've discussed this today, but I think is for the best to move this to 7026d. I intent to have one single ETF test round / one single final profiling round / one single final testing round. This will be easier if we will have this change in 7026d as well. Sorry for the inconvenience.

#6 Updated by Radu Apetrii about 3 years ago

Alexandru Lungu wrote:

Radu. I know we've discussed this today, but I think is for the best to move this to 7026d. I intent to have one single ETF test round / one single final profiling round / one single final testing round. This will be easier if we will have this change in 7026d as well. Sorry for the inconvenience.

No problem. I will get this done first thing in the morning. This way I can also handle that potential conflict in FieldReference.unknownValue by myself.

Apart from this, is there anything else that needs to be done regarding the code or is it clear enough?

#7 Updated by Alexandru Lungu about 3 years ago

Radu, I will review the changes once on 7026d.

#8 Updated by Radu Apetrii about 3 years ago

Done, I moved the changes. They are on 7026d, rev. 14577.

#9 Updated by Constantin Asofiei about 3 years ago

Radu, I've looked at BaseDataTypeFactory and:
  • please sort all the conditions alphabetically by the class name
  • the log warning should include the class name, and instead of Could not instantiate due to unknown class use Type ... instantiated via reflection, as it is not registered in BaseDataTypeFactory.

#10 Updated by Alexandru Lungu about 3 years ago

  • Status changed from WIP to Review
  • % Done changed from 80 to 100

I included this change with my FWD-H2 bump to version 1.20-trunk.

Therefore, I committed 7026d/rev. 14579 addressing #7365-9 and updating FWD-H2 to 1.20-trunk.

#11 Updated by Constantin Asofiei about 3 years ago

I think a type is missing, I get this in a customer app:

 Type class com.goldencode.p2j.util.ObjectVar instantiated via reflection, as it is not registered in BaseDataTypeFactory

#12 Updated by Radu Apetrii about 3 years ago

I've added a check for ObjectVar in BaseDataTypeFactory. The commit is on 7026e, rev. 14596.

#13 Updated by Alexandru Lungu about 3 years ago

  • Status changed from Review to WIP
  • % Done changed from 100 to 80

Radu, please check BufferFieldImpl.changeValue. Consider extending BaseDataTypeFactory with a new method to handle the following. This way we can avoid doing getConstructor and newInstance.

// try to invoke a polymorphic constructor of the field's type
Constructor<?> ctor = fieldRef.getType().getConstructor(BaseDataType.class);
bdt = (BaseDataType) ctor.newInstance(newValue); // TODO: OM: newValue is not compatible with fieldRef

#14 Updated by Alexandru Lungu about 3 years ago

Radu, if it doesn't take much time, please do a quick round-check of this issue and try to get this change in 7026e by the end of this week. Thank you.

#15 Updated by Radu Apetrii about 3 years ago

I added in BaseDataTypeFactory a function instantiate that also takes as a parameter the initial value that is to be used.
The commit is on 7026e, rev.14602.

Alexandru Lungu wrote:

Radu, please check BufferFieldImpl.changeValue. Consider extending BaseDataTypeFactory with a new method to handle the following. This way we can avoid doing getConstructor and newInstance.

I have refactored two such cases in BufferFieldImpl (the only ones I could find).

#16 Updated by Alexandru Lungu almost 3 years ago

  • Status changed from WIP to Review
  • % Done changed from 80 to 100

#17 Updated by Alexandru Lungu almost 3 years ago

  • Status changed from Review to Test

This was merged in trunk by 7026e and can be closed.

#18 Updated by Greg Shah almost 3 years ago

  • Status changed from Test to Closed

Also available in: Atom PDF