Skip to content

Switch identifiers to byte[] #4016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
headius wants to merge 3 commits into
base: main
Choose a base branch
from
+32 −24
Draft

Switch identifiers to byte[] #4016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8452788
Swich identifiers to byte[]
headius Mar 19, 2026
8ce1d14
Tweaks for byte[] identifiers
headius Mar 20, 2026
2129514
Remove no-longer-relevant comment
headius Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions templates/java/org/ruby_lang/prism/Loader.java.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%- string_type = Prism::Template::JAVA_STRING_TYPE -%>
<%- id_type = Prism::Template::JAVA_IDENTIFIER_TYPE -%>
package org.ruby_lang.prism;

import java.lang.Short;
Expand Down Expand Up @@ -27,29 +27,33 @@ public class Loader {
return Charset.forName(encodingName);
}

public <%= string_type %> bytesToName(byte[] bytes) {
<%- if string_type == "String" -%>
public <%= id_type %> bytesToName(byte[] bytes) {
<%- if id_type == "String" -%>
return new String(bytes, encodingCharset).intern();
<%- else -%>
return null; // Must be implemented by subclassing Loader
return bytes;
<%- end -%>
}

private static final class ConstantPool {

private final Loader loader;
private final int bufferOffset;
private final <%= string_type %>[] cache;
private final <%= id_type %>[] cache;

ConstantPool(Loader loader, int bufferOffset, int length) {
this.loader = loader;
this.bufferOffset = bufferOffset;
cache = new <%= string_type %>[length];
<%- if id_type == "String" -%>
cache = new <%= id_type %>[length];
<%- else -%>
cache = new byte[length][];
<%- end -%>
}

<%= string_type %> get(ByteBuffer buffer, int oneBasedIndex) {
<%= id_type %> get(ByteBuffer buffer, int oneBasedIndex) {
int index = oneBasedIndex - 1;
<%= string_type %> constant = cache[index];
<%= id_type %> constant = cache[index];

if (constant == null) {
int offset = bufferOffset + index * 8;
Expand All @@ -70,7 +74,7 @@ public class Loader {

private final ByteBuffer buffer;
protected String encodingName;
<%- if string_type == "String" -%>
<%- if id_type == "String" -%>
private Charset encodingCharset;
Copy link
Member

@eregon eregon Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think encodingCharset could be removed completely (as well as getEncodingCharset()) and then bytesToName would e.g. throw new AbstractMethodError(); for the id_type == "String" case, forcing it to be implemented in the subclass.
From #4018 (comment)

Copy link
Contributor Author

@headius headius Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be reasonable to me. I'll make that change before marking for review.

<%- end -%>
private ConstantPool constantPool;
Expand Down Expand Up @@ -100,7 +104,7 @@ public class Loader {
byte[] encodingNameBytes = new byte[encodingLength];
buffer.get(encodingNameBytes);
this.encodingName = new String(encodingNameBytes, StandardCharsets.US_ASCII);
<%- if string_type == "String" -%>
<%- if id_type == "String" -%>
this.encodingCharset = getEncodingCharset(this.encodingName);
<%- end -%>

Expand Down Expand Up @@ -213,11 +217,11 @@ public class Loader {
}
}

private <%= string_type %> loadConstant() {
private <%= id_type %> loadConstant() {
return constantPool.get(buffer, loadVarUInt());
}

private <%= string_type %> loadOptionalConstant() {
private <%= id_type %> loadOptionalConstant() {
if (buffer.get(buffer.position()) != 0) {
return loadConstant();
} else {
Expand All @@ -226,12 +230,16 @@ public class Loader {
}
}

private <%= string_type %>[] loadConstants() {
private <%= id_type %>[] loadConstants() {
int length = loadVarUInt();
if (length == 0) {
return Nodes.EMPTY_STRING_ARRAY;
return Nodes.EMPTY_IDENTIFIER_ARRAY;
}
<%= string_type %>[] constants = new <%= string_type %>[length];
<%- if id_type == "String" -%>
<%= id_type %>[] constants = new <%= id_type %>[length];
<%- else -%>
<%= id_type %>[] constants = new byte[length][];
<%- end -%>
for (int i = 0; i < length; i++) {
constants[i] = constantPool.get(buffer, loadVarUInt());
}
Expand Down Expand Up @@ -395,7 +403,7 @@ public class Loader {
int bufferPosition = buffer.position();
int serializedLength = buffer.getInt();
// Load everything except the body and locals, because the name, receiver, parameters are still needed for lazily defining the method
Nodes.DefNode lazyDefNode = new Nodes.DefNode(<%= base_params.join(", ") -%>, -bufferPosition, this, loadConstant(), loadOptionalNode(), (Nodes.ParametersNode) loadOptionalNode(), null, Nodes.EMPTY_STRING_ARRAY);
Nodes.DefNode lazyDefNode = new Nodes.DefNode(<%= base_params.join(", ") -%>, -bufferPosition, this, loadConstant(), loadOptionalNode(), (Nodes.ParametersNode) loadOptionalNode(), null, Nodes.EMPTY_IDENTIFIER_ARRAY);
buffer.position(bufferPosition + serializedLength); // skip past the serialized DefNode
return lazyDefNode;
}
Expand Down
6 changes: 3 additions & 3 deletions templates/java/org/ruby_lang/prism/Nodes.java.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%- string_type = Prism::Template::JAVA_STRING_TYPE -%>
<%- id_type = Prism::Template::JAVA_IDENTIFIER_TYPE -%>
package org.ruby_lang.prism;

import java.lang.Override;
Expand All @@ -16,7 +16,7 @@ import java.util.Arrays;
// @formatter:off
public abstract class Nodes {

public static final <%= string_type %>[] EMPTY_STRING_ARRAY = {};
public static final <%= id_type %>[] EMPTY_IDENTIFIER_ARRAY = {};

@Target(ElementType.FIELD)
@Retention(RetentionPolicy.SOURCE)
Expand Down Expand Up @@ -383,7 +383,7 @@ public abstract class Nodes {
builder.append('\n');
<%- when Prism::Template::ConstantListField -%>
builder.append('\n');
for (<%= string_type %> constant : this.<%= field.name %>) {
for (<%= id_type %> constant : this.<%= field.name %>) {
builder.append(nextNextIndent).append('"').append(constant).append('"').append('\n');
}
<%- when Prism::Template::Flags -%>
Expand Down
10 changes: 5 additions & 5 deletions templates/template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ module Template # :nodoc: all
REMOVE_ON_ERROR_TYPES = SERIALIZE_ONLY_SEMANTICS_FIELDS
CHECK_FIELD_KIND = ENV.fetch("CHECK_FIELD_KIND", false)

JAVA_BACKEND = ENV["PRISM_JAVA_BACKEND"] || "truffleruby"
JAVA_STRING_TYPE = JAVA_BACKEND == "jruby" ? "org.jruby.RubySymbol" : "String"
JAVA_BACKEND = ENV["PRISM_JAVA_BACKEND"] || "default"
JAVA_IDENTIFIER_TYPE = JAVA_BACKEND == "truffleruby" ? "String" : "byte[]"
INCLUDE_NODE_ID = !SERIALIZE_ONLY_SEMANTICS_FIELDS || JAVA_BACKEND == "jruby"

COMMON_FLAGS_COUNT = 2
Expand Down Expand Up @@ -272,7 +272,7 @@ def call_seq_type
end

def java_type
JAVA_STRING_TYPE
JAVA_IDENTIFIER_TYPE
end
end

Expand All @@ -292,7 +292,7 @@ def call_seq_type
end

def java_type
JAVA_STRING_TYPE
JAVA_IDENTIFIER_TYPE
end
end

Expand All @@ -312,7 +312,7 @@ def call_seq_type
end

def java_type
"#{JAVA_STRING_TYPE}[]"
"#{JAVA_IDENTIFIER_TYPE}[]"
end
end

Expand Down