Add field write stripping with @Environment #1047
Conversation
|
Does this mean that method calls and other related references may come also in the future? |
| } | ||
|
|
||
| @Override | ||
| public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) { | ||
| if (stripMethods.contains(name + descriptor)) return null; | ||
| return super.visitMethod(access, name, descriptor, signature, exceptions); | ||
| if (stripMethods.contains(name + descriptor)) { |
There was a problem hiding this comment.
More of a problem with the existing code than your changes but using fresh strings as hashtable keys is very slow because each one needs a fresh hashcode computation. Should be a record instead.
There was a problem hiding this comment.
we don't have records in this world :D
I'll probably overhaul the whole thing after the extension merge
There was a problem hiding this comment.
It can equally be a small regular class with a proper equals and hashCode.
There was a problem hiding this comment.
Of course, but it also touches at least 3 classes. For best performance it'd ideally be a specialized dual key set that can be queried without allocating anything.
There was a problem hiding this comment.
Sounds overkill, allocations are more or less free and this is especially elidable by the JIT. Point is allocating a class which holds both strings is far far cheaper than concatenation and a subsequent uncached hashCode computation.
There was a problem hiding this comment.
They are almost always latin 1 strings of short length, I don't see how that'd dominate the allocation and associated cleanup.
There was a problem hiding this comment.
Method descriptors can be very long. Regardless I'm not saying it's necessarily a real performance issue, just a code smell IMO
| MethodVisitor ret = super.visitMethod(access, name, descriptor, signature, exceptions); | ||
|
|
||
| // strip field writes from static init and constructor | ||
| if (!stripFields.isEmpty() && (name.equals("<clinit>") || name.equals("<init>"))) { |
There was a problem hiding this comment.
This will strip writes to other instances, will strip non-ctor writes to instance fields, but only if they're in <clinit>, and vice versa. Seems confusing and is mostly avoidable.
There was a problem hiding this comment.
Not sure that's a problem? It essentially pretends the field isn't there at all and there can't be both a static and non-static version of the same name+desc pair
There was a problem hiding this comment.
It seems odd for a non-static write to be stripped from <clinit> but not from any other non-ctor methods.
There was a problem hiding this comment.
I'm not too concerned about other methods since they aren't being populated by the inline field initializer syntax and scanning everything might be a bit too expensive. Restricting <clinit> to PUTSTATIC and <init> to PUTFIELD would be more effort while potentially leaving more "fixable" references that'd throw.
There was a problem hiding this comment.
I think stripping PUTSTATIC from clinit and PUTFIELD from init would be very little effort and make far more sense.
src/main/java/net/fabricmc/loader/impl/transformer/ClassStripper.java
Outdated
Show resolved
Hide resolved
Unlikely, this would be too much effort to implement and potentially too costly at runtime. The latter however might not matter much after implementing caching the final bytecode. |
This extends field stripping with stripping the field write from initializers. Usually the initializer itself is benign and only broke on the assignment with NoSuchFieldError like
This PR should resolve most incompatibilities introduced by #860 - which made field stripping work in the first place. If an initializer ran successfully before that PR, it'd work again with this PR unless there is a read to a stripped field involved. Initializers that accessed missing types or other elements were and remain unsupported.
Mods still shouldn't use
@Environmentover proper environment specific code separation, ideally with split client/common/server source sets.