Rewriting MetadataStorage into Java #8366
Conversation
JaroslavTulach
added
the
CI: No changelog needed
JaroslavTulach
requested review from
4e6,
hubertp and
Akirathan
as code owners
| @@ -187,7 +187,7 @@ case object AliasAnalysis extends IRPass { | |||
| case occ: Info.Occurrence => | |||
| occ.copy(graph = copyRootScopeGraph) | |||
| } | |||
| copyNode.updateMetadata(this -->> newMeta) | |||
| copyNode.updateMetadata(new MetadataPair(this, newMeta)) | |||
There was a problem hiding this comment.
Using normal new MetadataPair constructor instead of the fancy -->> operator. Majority of changes in this PR is related to this switch - at least in the passes.
| }); | ||
| out.writeInline(scala.collection.immutable.Map.class, map); | ||
| var map = new HashMap<String, Object>(); | ||
| obj.map( |
There was a problem hiding this comment.
.map no longer constructs scala.collections.immutable.Map, but just returns java.util.List. The .map operation isn't probably used anywhere than here else anyway.
| @@ -91,7 +88,7 @@ object Implicits { | |||
| * @return the metadata for `pass`, if it exists | |||
| */ | |||
| def getMetadata[K <: ProcessingPass](pass: K): Option[pass.Metadata] = { | |||
| ir.passData.get(pass) | |||
| ir.passData.get(pass).asInstanceOf[Option[pass.Metadata]] | |||
There was a problem hiding this comment.
The type-system of Java doesn't support "abstract types". Doing something like that in Java would require a bit of overhaul - easier to just cast at few places.
| @@ -48,7 +48,7 @@ object Name { | |||
| typePointer: Option[Name], | |||
| methodName: Name, | |||
| location: Option[IdentifiedLocation], | |||
| passData: MetadataStorage = MetadataStorage(), | |||
| passData: MetadataStorage = new MetadataStorage(), | |||
There was a problem hiding this comment.
There is no apply anymore. All the places are required to use new MetadataStorage. Another source of huge changes.
| * @tparam K the concrete type of `pass` | ||
| */ | ||
| public void update(ProcessingPass pass, ProcessingPass.Metadata newMeta) { | ||
| var copy = copyMetaMap(); |
There was a problem hiding this comment.
Why do we need to copy the map every time?
There was a problem hiding this comment.
That's what the original Scala implementation was doing as well - create new Map and reassign it to metadata.
There was a problem hiding this comment.
What's actually interesting is to compare:
- performance
- (multi threaded) consistency
@hubertp managed to get lazy immutable Scala map working. With that we can choose - do we want to use scala...immutable.Map here or immutable java.util.Map here? It is a valid question and I haven't yet chosen my answer.
| }; | ||
|
|
||
| private Map<ProcessingPass, ProcessingPass.Metadata> copyMetaMap() { | ||
| var copy = new TreeMap<ProcessingPass, ProcessingPass.Metadata>(COMPARATOR); |
There was a problem hiding this comment.
Do we care about the order of the entries?
There was a problem hiding this comment.
Yes, some tests do care. It might be enough to use LinkedHashMap, but just using HashMap doesn't work as that provides completely random order. Anyway the toString() implementation tries to sort the passes, so having TreeMap may be good idea to start with.
| @Override | ||
| public int hashCode() { | ||
| int hash = 5; | ||
| hash = 17 * hash + Objects.hashCode(this.metadata); |
There was a problem hiding this comment.
What is the reason for this custom hashCode? I have seen this somewhere else as well. Why not only delegate to Objects.hashCode(this.metadata)?
| * when serialized. | ||
| * | ||
| * Due to the type safety properties of | ||
| * [[org.enso.compiler.core.ir.MetadataStorage]], to allow this conversion |
There was a problem hiding this comment.
minor: convert scala doc to javadoc - replace stuff like [[ with {@code, etc.
Pull Request Description
Gets us closer to #8324 goal and replaces one core class written in Scala with a simplified Java implementation.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
style guides/blob/develop/docs/style-guide/rust.md) style guide.