Project

General

Profile

Feature #7126

replace 'new unknown()' with 'unknown.INSTANCE' in FWD conversion and runtime

Added by Constantin Asofiei over 3 years ago. Updated over 3 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Target version:
-
Start date:
Due date:
% Done:

0%

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

History

#1 Updated by Constantin Asofiei over 3 years ago

In 7026a rev 14488, new unknown() was replaced with unknown.INSTANCE in parts of FWD runtime which showed up in profiling a large customer application. We need to make this more general:
  • for conversion, any new unknown() must be converted to unknown.INSTANCE
  • for runtime, replace all new unknown() occurrences with unknown.INSTANCE

As an unknown instance is immutable, there is no reason to create new instances of it.

#2 Updated by Greg Shah over 3 years ago

Good idea. My only critique is that I want to find a different name than unknown.INSTANCE. I think this is a bit confusing.

How about if we add import static com.goldencode.p2j.util.unknown.UNKNOWN; and then simply reference UNKNOWN? We could also call it UNKNOWN_VALUE but I'd prefer the shorter text.

#3 Updated by Constantin Asofiei over 3 years ago

Greg Shah wrote:

How about if we add import static com.goldencode.p2j.util.unknown.UNKNOWN; and then simply reference UNKNOWN?

That works, too. I already have this unknown.INSTANCE name in 7026a, I'll rename it in 7026a. Btw, I'm not planing to work on this task at this time, as the other new unknown() references did not show in the profiler.

#4 Updated by Constantin Asofiei over 3 years ago

Renamed unknown.INSTANCE to unknown.UNKNOWN in 7026a/14493.

#5 Updated by Vladimir Tsichevski over 3 years ago

I can take this task.

A question: currently there is 3 ways to obtain the unknown value:

  1. calling new unknown(), we want to get rid of this method, OK. To make sure, just make this constructor private.
  2. referencing the unknown.UNKNOWN constant. I gather, this is the recommended method.
  3. calling the unknown.instantiateUnknown(), this function just returns the unknown.UNKNOWN constant. This method is called 8 times in the package, never referenced as a lambda.

I think, the last method should be eliminated either?

#6 Updated by Constantin Asofiei over 3 years ago

Vladimir Tsichevski wrote:

I can take this task.

Greg?

  1. calling new unknown(), we want to get rid of this method, OK. To make sure, just make this constructor private.

Correct, this will enforce the constant usage.

  1. referencing the unknown.UNKNOWN constant. I gather, this is the recommended method.

Correct.

  1. calling the unknown.instantiateUnknown(), this function just returns the unknown.UNKNOWN constant. This method is called 8 times in the package, never referenced as a lambda.

This needs to remain, this is an impl for the abstract method in BDT.

Also, all "unknown" references in the TRPL code need to be reviewed and replaced with unknown.UNKNOWN instead of new unknown().

#7 Updated by Vladimir Tsichevski over 3 years ago

Constantin Asofiei wrote:

  1. calling the unknown.instantiateUnknown(), this function just returns the unknown.UNKNOWN constant. This method is called 8 times in the package, never referenced as a lambda.

This needs to remain, this is an impl for the abstract method in BDT.

Now I see it, I have not noticed this due to the missing @Override annotation :-(

#8 Updated by Greg Shah over 3 years ago

This task isn't a priority right now.

Also available in: Atom PDF