From f763ed3bff5783e2859e9ff0d2391f55c45da896 Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 08:20:40 +0200 Subject: [PATCH 1/8] Remove setters from Decoder --- ext/java/org/msgpack/jruby/Decoder.java | 20 +++++++------------ .../org/msgpack/jruby/MessagePackLibrary.java | 8 +++++--- ext/java/org/msgpack/jruby/Unpacker.java | 12 +++-------- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Decoder.java b/ext/java/org/msgpack/jruby/Decoder.java index 09a02ac8..a3df5510 100644 --- a/ext/java/org/msgpack/jruby/Decoder.java +++ b/ext/java/org/msgpack/jruby/Decoder.java @@ -41,22 +41,22 @@ public class Decoder implements Iterator { private boolean allowUnknownExt; public Decoder(Ruby runtime) { - this(runtime, null, new byte[] {}, 0, 0); + this(runtime, null, new byte[] {}, 0, 0, false, false); } public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry) { - this(runtime, registry, new byte[] {}, 0, 0); + this(runtime, registry, new byte[] {}, 0, 0, false, false); } public Decoder(Ruby runtime, byte[] bytes) { - this(runtime, null, bytes, 0, bytes.length); + this(runtime, null, bytes, 0, bytes.length, false, false); } public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes) { - this(runtime, registry, bytes, 0, bytes.length); + this(runtime, registry, bytes, 0, bytes.length, false, false); } - public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes, int offset, int length) { + public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes, int offset, int length, boolean symbolizeKeys, boolean allowUnknownExt) { this.runtime = runtime; this.registry = registry; this.binaryEncoding = runtime.getEncodingService().getAscii8bitEncoding(); @@ -67,17 +67,11 @@ public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes, this.stackErrorClass = runtime.getModule("MessagePack").getClass("StackError"); this.unexpectedTypeErrorClass = runtime.getModule("MessagePack").getClass("UnexpectedTypeError"); this.unknownExtTypeErrorClass = runtime.getModule("MessagePack").getClass("UnknownExtTypeError"); + this.symbolizeKeys = symbolizeKeys; + this.allowUnknownExt = allowUnknownExt; feed(bytes, offset, length); } - public void symbolizeKeys(boolean symbolize) { - this.symbolizeKeys = symbolize; - } - - public void allowUnknownExt(boolean allow) { - this.allowUnknownExt = allow; - } - public void feed(byte[] bytes) { feed(bytes, 0, bytes.length); } diff --git a/ext/java/org/msgpack/jruby/MessagePackLibrary.java b/ext/java/org/msgpack/jruby/MessagePackLibrary.java index 358d8635..ade115f9 100644 --- a/ext/java/org/msgpack/jruby/MessagePackLibrary.java +++ b/ext/java/org/msgpack/jruby/MessagePackLibrary.java @@ -118,12 +118,14 @@ public static IRubyObject pack(ThreadContext ctx, IRubyObject recv, IRubyObject[ @JRubyMethod(module = true, required = 1, optional = 1, alias = {"load"}) public static IRubyObject unpack(ThreadContext ctx, IRubyObject recv, IRubyObject[] args) { Unpacker.ExtensionRegistry registry = MessagePackLibrary.defaultFactory.unpackerRegistry(); - Decoder decoder = new Decoder(ctx.getRuntime(), registry, args[0].asString().getBytes()); + boolean symbolizeKeys = false; if (args.length > 1 && !args[args.length - 1].isNil()) { RubyHash hash = args[args.length - 1].convertToHash(); - IRubyObject symbolizeKeys = hash.fastARef(ctx.getRuntime().newSymbol("symbolize_keys")); - decoder.symbolizeKeys(symbolizeKeys != null && symbolizeKeys.isTrue()); + IRubyObject symbolizeKeysVal = hash.fastARef(ctx.getRuntime().newSymbol("symbolize_keys")); + symbolizeKeys = symbolizeKeysVal != null && symbolizeKeysVal.isTrue(); } + byte[] bytes = args[0].asString().getBytes(); + Decoder decoder = new Decoder(ctx.getRuntime(), registry, bytes, 0, bytes.length, symbolizeKeys, false); return decoder.next(); } } diff --git a/ext/java/org/msgpack/jruby/Unpacker.java b/ext/java/org/msgpack/jruby/Unpacker.java index de007d03..9a87d94b 100644 --- a/ext/java/org/msgpack/jruby/Unpacker.java +++ b/ext/java/org/msgpack/jruby/Unpacker.java @@ -184,9 +184,7 @@ public IRubyObject executeLimit(ThreadContext ctx, IRubyObject str, IRubyObject if (limit == -1) { limit = byteList.length() - offset; } - Decoder decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin() + offset, limit); - decoder.symbolizeKeys(symbolizeKeys); - decoder.allowUnknownExt(allowUnknownExt); + Decoder decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin() + offset, limit, symbolizeKeys, allowUnknownExt); try { data = null; data = decoder.next(); @@ -216,9 +214,7 @@ public IRubyObject finished_p(ThreadContext ctx) { public IRubyObject feed(ThreadContext ctx, IRubyObject data) { ByteList byteList = data.asString().getByteList(); if (decoder == null) { - decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin(), byteList.length()); - decoder.symbolizeKeys(symbolizeKeys); - decoder.allowUnknownExt(allowUnknownExt); + decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin(), byteList.length(), symbolizeKeys, allowUnknownExt); } else { decoder.feed(byteList.unsafeBytes(), byteList.begin(), byteList.length()); } @@ -353,9 +349,7 @@ public IRubyObject setStream(ThreadContext ctx, IRubyObject stream) { ByteList byteList = str.getByteList(); this.stream = stream; this.decoder = null; - this.decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin(), byteList.length()); - decoder.symbolizeKeys(symbolizeKeys); - decoder.allowUnknownExt(allowUnknownExt); + this.decoder = new Decoder(ctx.getRuntime(), registry, byteList.unsafeBytes(), byteList.begin(), byteList.length(), symbolizeKeys, allowUnknownExt); return getStream(ctx); } } From 48f9ab589041c01b2d5a5a6ea6bbf6ab27a8c58c Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 08:28:48 +0200 Subject: [PATCH 2/8] Remove setter from Encoder --- ext/java/org/msgpack/jruby/Encoder.java | 7 ++----- ext/java/org/msgpack/jruby/Packer.java | 15 +++++---------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Encoder.java b/ext/java/org/msgpack/jruby/Encoder.java index 7deef769..182a6a14 100644 --- a/ext/java/org/msgpack/jruby/Encoder.java +++ b/ext/java/org/msgpack/jruby/Encoder.java @@ -34,19 +34,16 @@ public class Encoder { private final Encoding binaryEncoding; private final Encoding utf8Encoding; private final boolean compatibilityMode; + private final Packer.ExtensionRegistry registry; - private Packer.ExtensionRegistry registry; private ByteBuffer buffer; - public Encoder(Ruby runtime, boolean compatibilityMode) { + public Encoder(Ruby runtime, boolean compatibilityMode, Packer.ExtensionRegistry registry) { this.runtime = runtime; this.buffer = ByteBuffer.allocate(CACHE_LINE_SIZE - ARRAY_HEADER_SIZE); this.binaryEncoding = runtime.getEncodingService().getAscii8bitEncoding(); this.utf8Encoding = UTF8Encoding.INSTANCE; this.compatibilityMode = compatibilityMode; - } - - public void setRegistry(Packer.ExtensionRegistry registry) { this.registry = registry; } diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index cbfe66d7..916b7deb 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -25,13 +25,14 @@ public class Packer extends RubyObject { private Buffer buffer; private Encoder encoder; - public Packer(Ruby runtime, RubyClass type) { + public Packer(Ruby runtime, RubyClass type, ExtensionRegistry registry) { super(runtime, type); + this.registry = registry; } static class PackerAllocator implements ObjectAllocator { public IRubyObject allocate(Ruby runtime, RubyClass type) { - return new Packer(runtime, type); + return new Packer(runtime, type, null); } } @@ -104,22 +105,16 @@ public IRubyObject initialize(ThreadContext ctx, IRubyObject[] args) { IRubyObject mode = options.fastARef(ctx.getRuntime().newSymbol("compatibility_mode")); compatibilityMode = (mode != null) && mode.isTrue(); } - this.encoder = new Encoder(ctx.getRuntime(), compatibilityMode); + this.encoder = new Encoder(ctx.getRuntime(), compatibilityMode, registry); this.buffer = new Buffer(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Buffer")); this.buffer.initialize(ctx, args); this.registry = new ExtensionRegistry(ctx.getRuntime()); return this; } - public void setExtensionRegistry(ExtensionRegistry registry) { - this.registry = registry; - this.encoder.setRegistry(registry); - } - public static Packer newPacker(ThreadContext ctx, ExtensionRegistry extRegistry, IRubyObject[] args) { - Packer packer = new Packer(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Packer")); + Packer packer = new Packer(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Packer"), extRegistry); packer.initialize(ctx, args); - packer.setExtensionRegistry(extRegistry); return packer; } From ac21a805c6ed390f8aad8a2620e3e604131cef9f Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 08:34:17 +0200 Subject: [PATCH 3/8] Remove comments Comments are at best outdated, but most of the time they confuse more than they help. Whenever you feel the urge to write a comment, think about how you can rewrite the code to make it not need a comment to explain what it does. --- ext/java/org/msgpack/jruby/Packer.java | 2 -- ext/java/org/msgpack/jruby/Unpacker.java | 3 --- 2 files changed, 5 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index 916b7deb..1198a6a0 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -130,8 +130,6 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) { @JRubyMethod(name = "register_type", required = 2, optional = 1) public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Block block) { - // register_type(type, Class){|obj| how_to_serialize.... } - // register_type(type, Class, :to_msgpack_ext) Ruby runtime = ctx.getRuntime(); IRubyObject type = args[0]; IRubyObject klass = args[1]; diff --git a/ext/java/org/msgpack/jruby/Unpacker.java b/ext/java/org/msgpack/jruby/Unpacker.java index 9a87d94b..744b19f1 100644 --- a/ext/java/org/msgpack/jruby/Unpacker.java +++ b/ext/java/org/msgpack/jruby/Unpacker.java @@ -69,7 +69,6 @@ public void put(RubyClass klass, int typeId, IRubyObject proc, IRubyObject arg) array[typeId + 128] = e; } - // proc, typeId(Fixnum) public IRubyObject lookup(int type) { RubyArray e = array[type + 128]; if (e == null) { @@ -136,8 +135,6 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) { @JRubyMethod(name = "register_type", required = 1, optional = 2) public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Block block) { - // register_type(type){|data| ExtClass.deserialize(...) } - // register_type(type, Class, :from_msgpack_ext) Ruby runtime = ctx.getRuntime(); IRubyObject type = args[0]; From 230b31321581d0bd9db6275609e914439910b6c7 Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 08:45:43 +0200 Subject: [PATCH 4/8] Replace a comment with a method By extracting the code below a comment to a method that explains what the code does the comment is not needed and the code is self-explaining. --- ext/java/org/msgpack/jruby/Packer.java | 30 +++++++++++++++----------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index 1198a6a0..4861871e 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -71,29 +71,33 @@ public IRubyObject[] lookup(final RubyClass klass) { hit[1] = e.entry(0); return hit; } + IRubyObject[] pair = findEntryByClassOrAncestor(hash, klass); + if (pair != null) { + cache.fastASet(pair[0], RubyArray.newArray(runtime, pair)); + } + return pair; + } + private IRubyObject[] findEntryByClassOrAncestor(final RubyHash hash, final RubyClass klass) { final IRubyObject[] pair = new IRubyObject[2]; - // check all keys whether it's super class of klass, or not hash.visitAll(new RubyHash.Visitor() { public void visit(IRubyObject keyValue, IRubyObject value) { - if (pair[0] != null) { - return; - } - ThreadContext ctx = runtime.getCurrentContext(); - RubyArray ancestors = (RubyArray) klass.callMethod(ctx, "ancestors"); - if (ancestors.callMethod(ctx, "include?", keyValue).isTrue()) { - RubyArray hit = (RubyArray) hash.fastARef(keyValue); - cache.fastASet(klass, hit); - pair[0] = hit.entry(1); - pair[1] = hit.entry(0); + if (pair[0] == null) { + ThreadContext ctx = runtime.getCurrentContext(); + RubyArray ancestors = (RubyArray) klass.callMethod(ctx, "ancestors"); + if (ancestors.callMethod(ctx, "include?", keyValue).isTrue()) { + RubyArray hit = (RubyArray) hash.fastARef(keyValue); + pair[0] = hit.entry(1); + pair[1] = hit.entry(0); + } } } }); - if (pair[0] == null) { return null; + } else { + return pair; } - return pair; } } From cc5a0198e8b8ada72d4d02d1779c64ca074809ae Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 08:48:49 +0200 Subject: [PATCH 5/8] Simplify array construction in Packer.ExtensionRegistry --- ext/java/org/msgpack/jruby/Packer.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index 4861871e..9d0257f9 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -66,10 +66,7 @@ public IRubyObject[] lookup(final RubyClass klass) { e = (RubyArray) cache.fastARef(klass); } if (e != null) { - IRubyObject[] hit = new IRubyObject[2]; - hit[0] = e.entry(1); - hit[1] = e.entry(0); - return hit; + return new IRubyObject[] {e.entry(1), e.entry(0)}; } IRubyObject[] pair = findEntryByClassOrAncestor(hash, klass); if (pair != null) { From 18e070e7c23d5476bbcdc7644a959a598519f63a Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 09:02:16 +0200 Subject: [PATCH 6/8] Remove an unnecessary comment What this comment says is better explained by the exception thrown by the method. --- ext/java/org/msgpack/jruby/Unpacker.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Unpacker.java b/ext/java/org/msgpack/jruby/Unpacker.java index 744b19f1..716a8b75 100644 --- a/ext/java/org/msgpack/jruby/Unpacker.java +++ b/ext/java/org/msgpack/jruby/Unpacker.java @@ -278,10 +278,6 @@ public IRubyObject read(ThreadContext ctx) { } } - /* - skip & skip_nil don't exist in JRuby implementation: - these depend on CRuby msgpack implemntation too highly. - */ @JRubyMethod(name = "skip") public IRubyObject skip(ThreadContext ctx) { throw ctx.getRuntime().newNotImplementedError("Not supported yet in JRuby implementation"); From 1e9ba4076772eb75ae68878761ec0e86b191028a Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 09:43:37 +0200 Subject: [PATCH 7/8] Extract Packer.ExtensionRegistry to its own class and clean up the internals Use Java collections for the registry and cache, and use a class for the four-tuple containing the registered entries. --- ext/java/org/msgpack/jruby/Encoder.java | 4 +- .../org/msgpack/jruby/ExtensionRegistry.java | 98 +++++++++++++++++++ ext/java/org/msgpack/jruby/Factory.java | 8 +- ext/java/org/msgpack/jruby/Packer.java | 66 +------------ 4 files changed, 106 insertions(+), 70 deletions(-) create mode 100644 ext/java/org/msgpack/jruby/ExtensionRegistry.java diff --git a/ext/java/org/msgpack/jruby/Encoder.java b/ext/java/org/msgpack/jruby/Encoder.java index 182a6a14..879b75c0 100644 --- a/ext/java/org/msgpack/jruby/Encoder.java +++ b/ext/java/org/msgpack/jruby/Encoder.java @@ -34,11 +34,11 @@ public class Encoder { private final Encoding binaryEncoding; private final Encoding utf8Encoding; private final boolean compatibilityMode; - private final Packer.ExtensionRegistry registry; + private final ExtensionRegistry registry; private ByteBuffer buffer; - public Encoder(Ruby runtime, boolean compatibilityMode, Packer.ExtensionRegistry registry) { + public Encoder(Ruby runtime, boolean compatibilityMode, ExtensionRegistry registry) { this.runtime = runtime; this.buffer = ByteBuffer.allocate(CACHE_LINE_SIZE - ARRAY_HEADER_SIZE); this.binaryEncoding = runtime.getEncodingService().getAscii8bitEncoding(); diff --git a/ext/java/org/msgpack/jruby/ExtensionRegistry.java b/ext/java/org/msgpack/jruby/ExtensionRegistry.java new file mode 100644 index 00000000..a8b63e65 --- /dev/null +++ b/ext/java/org/msgpack/jruby/ExtensionRegistry.java @@ -0,0 +1,98 @@ +package org.msgpack.jruby; + +import org.jruby.Ruby; +import org.jruby.RubyHash; +import org.jruby.RubyArray; +import org.jruby.RubyClass; +import org.jruby.RubyFixnum; +import org.jruby.runtime.ThreadContext; +import org.jruby.runtime.builtin.IRubyObject; + +import java.util.Map; +import java.util.HashMap; + +public class ExtensionRegistry { + private final Map extensionsByClass; + private final Map extensionsByAncestor; + + public ExtensionRegistry() { + this(new HashMap()); + } + + private ExtensionRegistry(Map extensionsByClass) { + this.extensionsByClass = new HashMap(extensionsByClass); + this.extensionsByAncestor = new HashMap(); + } + + public ExtensionRegistry dup() { + return new ExtensionRegistry(extensionsByClass); + } + + public IRubyObject toRubyHash(ThreadContext ctx) { + RubyHash hash = RubyHash.newHash(ctx.getRuntime()); + for (RubyClass extensionClass : extensionsByClass.keySet()) { + hash.put(extensionClass, extensionsByClass.get(extensionClass).toTuple(ctx)); + } + return hash; + } + + public void put(RubyClass cls, int typeId, IRubyObject proc, IRubyObject arg) { + extensionsByClass.put(cls, new ExtensionEntry(cls, typeId, proc, arg)); + extensionsByAncestor.clear(); + } + + public IRubyObject[] lookup(RubyClass cls) { + ExtensionEntry e = extensionsByClass.get(cls); + if (e == null) { + e = extensionsByAncestor.get(cls); + } + if (e == null) { + e = findEntryByClassOrAncestor(cls); + if (e != null) { + extensionsByAncestor.put(e.getExtensionClass(), e); + } + } + if (e == null) { + return null; + } else { + return e.toProcTypePair(cls.getRuntime().getCurrentContext()); + } + } + + private ExtensionEntry findEntryByClassOrAncestor(final RubyClass cls) { + ThreadContext ctx = cls.getRuntime().getCurrentContext(); + for (RubyClass extensionClass : extensionsByClass.keySet()) { + RubyArray ancestors = (RubyArray) cls.callMethod(ctx, "ancestors"); + if (ancestors.callMethod(ctx, "include?", extensionClass).isTrue()) { + return extensionsByClass.get(extensionClass); + } + } + return null; + } + + private static class ExtensionEntry { + private final RubyClass cls; + private final int typeId; + private final IRubyObject proc; + private final IRubyObject arg; + + public ExtensionEntry(RubyClass cls, int typeId, IRubyObject proc, IRubyObject arg) { + this.cls = cls; + this.typeId = typeId; + this.proc = proc; + this.arg = arg; + } + + public RubyClass getExtensionClass() { + return cls; + } + + public RubyArray toTuple(ThreadContext ctx) { + return RubyArray.newArray(ctx.getRuntime(), new IRubyObject[] {RubyFixnum.newFixnum(ctx.getRuntime(), typeId), proc, arg}); + } + + public IRubyObject[] toProcTypePair(ThreadContext ctx) { + return new IRubyObject[] {proc, RubyFixnum.newFixnum(ctx.getRuntime(), typeId)}; + } + } +} diff --git a/ext/java/org/msgpack/jruby/Factory.java b/ext/java/org/msgpack/jruby/Factory.java index f494dd5a..99c14b01 100644 --- a/ext/java/org/msgpack/jruby/Factory.java +++ b/ext/java/org/msgpack/jruby/Factory.java @@ -23,7 +23,7 @@ @JRubyClass(name="MessagePack::Factory") public class Factory extends RubyObject { private Ruby runtime; - private Packer.ExtensionRegistry packerExtensionRegistry; + private ExtensionRegistry packerExtensionRegistry; private Unpacker.ExtensionRegistry unpackerExtensionRegistry; public Factory(Ruby runtime, RubyClass type) { @@ -39,12 +39,12 @@ public IRubyObject allocate(Ruby runtime, RubyClass type) { @JRubyMethod(name = "initialize") public IRubyObject initialize(ThreadContext ctx) { - this.packerExtensionRegistry = new Packer.ExtensionRegistry(ctx.getRuntime()); + this.packerExtensionRegistry = new ExtensionRegistry(); this.unpackerExtensionRegistry = new Unpacker.ExtensionRegistry(ctx.getRuntime()); return this; } - public Packer.ExtensionRegistry packerRegistry() { + public ExtensionRegistry packerRegistry() { return this.packerExtensionRegistry.dup(); } @@ -72,7 +72,7 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) { } } - IRubyObject[] ary = { packerRegistry().hash, mapping }; + IRubyObject[] ary = { packerExtensionRegistry.toRubyHash(ctx), mapping }; return RubyArray.newArray(ctx.getRuntime(), ary); } diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index 9d0257f9..3fd47537 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -36,68 +36,6 @@ public IRubyObject allocate(Ruby runtime, RubyClass type) { } } - static class ExtensionRegistry { - private Ruby runtime; - public RubyHash hash; - public RubyHash cache; - - public ExtensionRegistry(Ruby runtime) { - this.runtime = runtime; - hash = RubyHash.newHash(runtime); - cache = RubyHash.newHash(runtime); - } - - public ExtensionRegistry dup() { - ExtensionRegistry copy = new ExtensionRegistry(runtime); - copy.hash = (RubyHash) hash.dup(runtime.getCurrentContext()); - copy.cache = RubyHash.newHash(runtime); - return copy; - } - - public void put(RubyClass klass, int typeId, IRubyObject proc, IRubyObject arg) { - RubyArray e = RubyArray.newArray(runtime, new IRubyObject[] { RubyFixnum.newFixnum(runtime, typeId), proc, arg }); - cache.rb_clear(); - hash.fastASet(klass, e); - } - - public IRubyObject[] lookup(final RubyClass klass) { - RubyArray e = (RubyArray) hash.fastARef(klass); - if (e == null) { - e = (RubyArray) cache.fastARef(klass); - } - if (e != null) { - return new IRubyObject[] {e.entry(1), e.entry(0)}; - } - IRubyObject[] pair = findEntryByClassOrAncestor(hash, klass); - if (pair != null) { - cache.fastASet(pair[0], RubyArray.newArray(runtime, pair)); - } - return pair; - } - - private IRubyObject[] findEntryByClassOrAncestor(final RubyHash hash, final RubyClass klass) { - final IRubyObject[] pair = new IRubyObject[2]; - hash.visitAll(new RubyHash.Visitor() { - public void visit(IRubyObject keyValue, IRubyObject value) { - if (pair[0] == null) { - ThreadContext ctx = runtime.getCurrentContext(); - RubyArray ancestors = (RubyArray) klass.callMethod(ctx, "ancestors"); - if (ancestors.callMethod(ctx, "include?", keyValue).isTrue()) { - RubyArray hit = (RubyArray) hash.fastARef(keyValue); - pair[0] = hit.entry(1); - pair[1] = hit.entry(0); - } - } - } - }); - if (pair[0] == null) { - return null; - } else { - return pair; - } - } - } - @JRubyMethod(name = "initialize", optional = 2) public IRubyObject initialize(ThreadContext ctx, IRubyObject[] args) { boolean compatibilityMode = false; @@ -109,7 +47,7 @@ public IRubyObject initialize(ThreadContext ctx, IRubyObject[] args) { this.encoder = new Encoder(ctx.getRuntime(), compatibilityMode, registry); this.buffer = new Buffer(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Buffer")); this.buffer.initialize(ctx, args); - this.registry = new ExtensionRegistry(ctx.getRuntime()); + this.registry = new ExtensionRegistry(); return this; } @@ -126,7 +64,7 @@ public IRubyObject isCompatibilityMode(ThreadContext ctx) { @JRubyMethod(name = "registered_types_internal", visibility = PRIVATE) public IRubyObject registeredTypesInternal(ThreadContext ctx) { - return registry.hash.dup(ctx); + return registry.toRubyHash(ctx); } @JRubyMethod(name = "register_type", required = 2, optional = 1) From 9b76228a8d3f3a18d3a8364f2d697b7128d81de8 Mon Sep 17 00:00:00 2001 From: Theo Date: Fri, 23 Oct 2015 11:03:31 +0200 Subject: [PATCH 8/8] Replace Unpacker.ExtensionRegistry with ExtensionRegistry Make it possible to look up extensions in ExtensionRegistry by type ID, and add the features needed to make it work for the unpacking side of things. Eliminate duplicated code in Factory#registered_types_internal and Unpacker#registered_types_internal by moving it to ExtensionRegistry. There are a few tests that fail because of this change. The reason is that Factory#registered_types_internal, Packer#registered_types_internal and Unpacker#registered_types_internal have inconsistent interfaces. The consumers of these methods expect slightly different things in the returned values. I consider this a bug, but I can't change it without changing the C code, something I'm not comfortable with. --- ext/java/org/msgpack/jruby/Decoder.java | 10 +- ext/java/org/msgpack/jruby/Encoder.java | 2 +- .../org/msgpack/jruby/ExtensionRegistry.java | 94 +++++++++++++++---- ext/java/org/msgpack/jruby/Factory.java | 41 +++----- .../org/msgpack/jruby/MessagePackLibrary.java | 2 +- ext/java/org/msgpack/jruby/Packer.java | 4 +- ext/java/org/msgpack/jruby/Unpacker.java | 59 ++---------- 7 files changed, 110 insertions(+), 102 deletions(-) diff --git a/ext/java/org/msgpack/jruby/Decoder.java b/ext/java/org/msgpack/jruby/Decoder.java index a3df5510..62f56511 100644 --- a/ext/java/org/msgpack/jruby/Decoder.java +++ b/ext/java/org/msgpack/jruby/Decoder.java @@ -35,7 +35,7 @@ public class Decoder implements Iterator { private final RubyClass unexpectedTypeErrorClass; private final RubyClass unknownExtTypeErrorClass; - private Unpacker.ExtensionRegistry registry; + private ExtensionRegistry registry; private ByteBuffer buffer; private boolean symbolizeKeys; private boolean allowUnknownExt; @@ -44,7 +44,7 @@ public Decoder(Ruby runtime) { this(runtime, null, new byte[] {}, 0, 0, false, false); } - public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry) { + public Decoder(Ruby runtime, ExtensionRegistry registry) { this(runtime, registry, new byte[] {}, 0, 0, false, false); } @@ -52,11 +52,11 @@ public Decoder(Ruby runtime, byte[] bytes) { this(runtime, null, bytes, 0, bytes.length, false, false); } - public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes) { + public Decoder(Ruby runtime, ExtensionRegistry registry, byte[] bytes) { this(runtime, registry, bytes, 0, bytes.length, false, false); } - public Decoder(Ruby runtime, Unpacker.ExtensionRegistry registry, byte[] bytes, int offset, int length, boolean symbolizeKeys, boolean allowUnknownExt) { + public Decoder(Ruby runtime, ExtensionRegistry registry, byte[] bytes, int offset, int length, boolean symbolizeKeys, boolean allowUnknownExt) { this.runtime = runtime; this.registry = registry; this.binaryEncoding = runtime.getEncodingService().getAscii8bitEncoding(); @@ -136,7 +136,7 @@ private IRubyObject consumeExtension(int size) { byte[] payload = readBytes(size); if (registry != null) { - IRubyObject proc = registry.lookup(type); + IRubyObject proc = registry.lookupUnpackerByTypeId(type); if (proc != null) { ByteList byteList = new ByteList(payload, runtime.getEncodingService().getAscii8bitEncoding()); return proc.callMethod(runtime.getCurrentContext(), "call", runtime.newString(byteList)); diff --git a/ext/java/org/msgpack/jruby/Encoder.java b/ext/java/org/msgpack/jruby/Encoder.java index 879b75c0..f803b849 100644 --- a/ext/java/org/msgpack/jruby/Encoder.java +++ b/ext/java/org/msgpack/jruby/Encoder.java @@ -350,7 +350,7 @@ private void appendExtensionValue(ExtensionValue object) { private void appendOther(IRubyObject object, IRubyObject destination) { if (registry != null) { - IRubyObject[] pair = registry.lookup(object.getType()); + IRubyObject[] pair = registry.lookupPackerByClass(object.getType()); if (pair != null) { RubyString bytes = pair[0].callMethod(runtime.getCurrentContext(), "call", object).asString(); int type = (int) ((RubyFixnum) pair[1]).getLongValue(); diff --git a/ext/java/org/msgpack/jruby/ExtensionRegistry.java b/ext/java/org/msgpack/jruby/ExtensionRegistry.java index a8b63e65..aa49b607 100644 --- a/ext/java/org/msgpack/jruby/ExtensionRegistry.java +++ b/ext/java/org/msgpack/jruby/ExtensionRegistry.java @@ -14,6 +14,7 @@ public class ExtensionRegistry { private final Map extensionsByClass; private final Map extensionsByAncestor; + private final ExtensionEntry[] extensionsByTypeId; public ExtensionRegistry() { this(new HashMap()); @@ -22,26 +23,57 @@ public ExtensionRegistry() { private ExtensionRegistry(Map extensionsByClass) { this.extensionsByClass = new HashMap(extensionsByClass); this.extensionsByAncestor = new HashMap(); + this.extensionsByTypeId = new ExtensionEntry[256]; + for (ExtensionEntry entry : extensionsByClass.values()) { + if (entry.hasUnpacker()) { + extensionsByTypeId[entry.getTypeId() + 128] = entry; + } + } } public ExtensionRegistry dup() { return new ExtensionRegistry(extensionsByClass); } - public IRubyObject toRubyHash(ThreadContext ctx) { + public IRubyObject toInternalPackerRegistry(ThreadContext ctx) { RubyHash hash = RubyHash.newHash(ctx.getRuntime()); for (RubyClass extensionClass : extensionsByClass.keySet()) { - hash.put(extensionClass, extensionsByClass.get(extensionClass).toTuple(ctx)); + ExtensionEntry entry = extensionsByClass.get(extensionClass); + if (entry.hasPacker()) { + hash.put(extensionClass, entry.toPackerTuple(ctx)); + } } return hash; } - public void put(RubyClass cls, int typeId, IRubyObject proc, IRubyObject arg) { - extensionsByClass.put(cls, new ExtensionEntry(cls, typeId, proc, arg)); + public IRubyObject toInternalUnpackerRegistry(ThreadContext ctx) { + RubyHash hash = RubyHash.newHash(ctx.getRuntime()); + for (ExtensionEntry entry : extensionsByClass.values()) { + IRubyObject typeId = RubyFixnum.newFixnum(ctx.getRuntime(), entry.getTypeId()); + if (entry.hasUnpacker()) { + hash.put(typeId, entry.toUnpackerTuple(ctx)); + } + } + return hash; + } + + public void put(RubyClass cls, int typeId, IRubyObject packerProc, IRubyObject packerArg, IRubyObject unpackerProc, IRubyObject unpackerArg) { + ExtensionEntry entry = new ExtensionEntry(cls, typeId, packerProc, packerArg, unpackerProc, unpackerArg); + extensionsByClass.put(cls, entry); + extensionsByTypeId[typeId + 128] = entry; extensionsByAncestor.clear(); } - public IRubyObject[] lookup(RubyClass cls) { + public IRubyObject lookupUnpackerByTypeId(int typeId) { + ExtensionEntry e = extensionsByTypeId[typeId + 128]; + if (e != null && e.hasUnpacker()) { + return e.getUnpackerProc(); + } else { + return null; + } + } + + public IRubyObject[] lookupPackerByClass(RubyClass cls) { ExtensionEntry e = extensionsByClass.get(cls); if (e == null) { e = extensionsByAncestor.get(cls); @@ -52,10 +84,10 @@ public IRubyObject[] lookup(RubyClass cls) { extensionsByAncestor.put(e.getExtensionClass(), e); } } - if (e == null) { - return null; + if (e != null && e.hasPacker()) { + return e.toPackerProcTypeIdPair(cls.getRuntime().getCurrentContext()); } else { - return e.toProcTypePair(cls.getRuntime().getCurrentContext()); + return null; } } @@ -73,26 +105,54 @@ private ExtensionEntry findEntryByClassOrAncestor(final RubyClass cls) { private static class ExtensionEntry { private final RubyClass cls; private final int typeId; - private final IRubyObject proc; - private final IRubyObject arg; + private final IRubyObject packerProc; + private final IRubyObject packerArg; + private final IRubyObject unpackerProc; + private final IRubyObject unpackerArg; - public ExtensionEntry(RubyClass cls, int typeId, IRubyObject proc, IRubyObject arg) { + public ExtensionEntry(RubyClass cls, int typeId, IRubyObject packerProc, IRubyObject packerArg, IRubyObject unpackerProc, IRubyObject unpackerArg) { this.cls = cls; this.typeId = typeId; - this.proc = proc; - this.arg = arg; + this.packerProc = packerProc; + this.packerArg = packerArg; + this.unpackerProc = unpackerProc; + this.unpackerArg = unpackerArg; } public RubyClass getExtensionClass() { return cls; } - public RubyArray toTuple(ThreadContext ctx) { - return RubyArray.newArray(ctx.getRuntime(), new IRubyObject[] {RubyFixnum.newFixnum(ctx.getRuntime(), typeId), proc, arg}); + public int getTypeId() { + return typeId; + } + + public boolean hasPacker() { + return packerProc != null; + } + + public boolean hasUnpacker() { + return unpackerProc != null; + } + + public IRubyObject getPackerProc() { + return packerProc; + } + + public IRubyObject getUnpackerProc() { + return unpackerProc; + } + + public RubyArray toPackerTuple(ThreadContext ctx) { + return RubyArray.newArray(ctx.getRuntime(), new IRubyObject[] {cls, packerProc, packerArg}); + } + + public RubyArray toUnpackerTuple(ThreadContext ctx) { + return RubyArray.newArray(ctx.getRuntime(), new IRubyObject[] {RubyFixnum.newFixnum(ctx.getRuntime(), typeId), unpackerProc, unpackerArg}); } - public IRubyObject[] toProcTypePair(ThreadContext ctx) { - return new IRubyObject[] {proc, RubyFixnum.newFixnum(ctx.getRuntime(), typeId)}; + public IRubyObject[] toPackerProcTypeIdPair(ThreadContext ctx) { + return new IRubyObject[] {packerProc, RubyFixnum.newFixnum(ctx.getRuntime(), typeId)}; } } } diff --git a/ext/java/org/msgpack/jruby/Factory.java b/ext/java/org/msgpack/jruby/Factory.java index 99c14b01..e5f5d0be 100644 --- a/ext/java/org/msgpack/jruby/Factory.java +++ b/ext/java/org/msgpack/jruby/Factory.java @@ -22,13 +22,13 @@ @JRubyClass(name="MessagePack::Factory") public class Factory extends RubyObject { - private Ruby runtime; - private ExtensionRegistry packerExtensionRegistry; - private Unpacker.ExtensionRegistry unpackerExtensionRegistry; + private final Ruby runtime; + private final ExtensionRegistry extensionRegistry; public Factory(Ruby runtime, RubyClass type) { super(runtime, type); this.runtime = runtime; + this.extensionRegistry = new ExtensionRegistry(); } static class FactoryAllocator implements ObjectAllocator { @@ -37,43 +37,31 @@ public IRubyObject allocate(Ruby runtime, RubyClass type) { } } + public ExtensionRegistry extensionRegistry() { + return extensionRegistry.dup(); + } + @JRubyMethod(name = "initialize") public IRubyObject initialize(ThreadContext ctx) { - this.packerExtensionRegistry = new ExtensionRegistry(); - this.unpackerExtensionRegistry = new Unpacker.ExtensionRegistry(ctx.getRuntime()); return this; } - public ExtensionRegistry packerRegistry() { - return this.packerExtensionRegistry.dup(); - } - - public Unpacker.ExtensionRegistry unpackerRegistry() { - return this.unpackerExtensionRegistry.dup(); - } - @JRubyMethod(name = "packer", optional = 1) public Packer packer(ThreadContext ctx, IRubyObject[] args) { - return Packer.newPacker(ctx, packerRegistry(), args); + return Packer.newPacker(ctx, extensionRegistry(), args); } @JRubyMethod(name = "unpacker", optional = 1) public Unpacker unpacker(ThreadContext ctx, IRubyObject[] args) { - return Unpacker.newUnpacker(ctx, unpackerRegistry(), args); + return Unpacker.newUnpacker(ctx, extensionRegistry(), args); } @JRubyMethod(name = "registered_types_internal", visibility = PRIVATE) public IRubyObject registeredTypesInternal(ThreadContext ctx) { - Unpacker.ExtensionRegistry reg = unpackerRegistry(); - RubyHash mapping = RubyHash.newHash(ctx.getRuntime()); - for (int i = 0; i < 256; i++) { - if (reg.array[i] != null) { - mapping.fastASet(RubyFixnum.newFixnum(ctx.getRuntime(), i - 128), reg.array[i]); - } - } - - IRubyObject[] ary = { packerExtensionRegistry.toRubyHash(ctx), mapping }; - return RubyArray.newArray(ctx.getRuntime(), ary); + return RubyArray.newArray(ctx.getRuntime(), new IRubyObject[] { + extensionRegistry.toInternalPackerRegistry(ctx), + extensionRegistry.toInternalUnpackerRegistry(ctx) + }); } @JRubyMethod(name = "register_type", required = 2, optional = 1) @@ -122,8 +110,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) { } } - this.packerExtensionRegistry.put(extClass, (int) typeId, packerProc, packerArg); - this.unpackerExtensionRegistry.put(extClass, (int) typeId, unpackerProc, unpackerArg); + extensionRegistry.put(extClass, (int) typeId, packerProc, packerArg, unpackerProc, unpackerArg); return runtime.getNil(); } diff --git a/ext/java/org/msgpack/jruby/MessagePackLibrary.java b/ext/java/org/msgpack/jruby/MessagePackLibrary.java index ade115f9..78ae1e34 100644 --- a/ext/java/org/msgpack/jruby/MessagePackLibrary.java +++ b/ext/java/org/msgpack/jruby/MessagePackLibrary.java @@ -117,7 +117,7 @@ public static IRubyObject pack(ThreadContext ctx, IRubyObject recv, IRubyObject[ @JRubyMethod(module = true, required = 1, optional = 1, alias = {"load"}) public static IRubyObject unpack(ThreadContext ctx, IRubyObject recv, IRubyObject[] args) { - Unpacker.ExtensionRegistry registry = MessagePackLibrary.defaultFactory.unpackerRegistry(); + ExtensionRegistry registry = MessagePackLibrary.defaultFactory.extensionRegistry(); boolean symbolizeKeys = false; if (args.length > 1 && !args[args.length - 1].isNil()) { RubyHash hash = args[args.length - 1].convertToHash(); diff --git a/ext/java/org/msgpack/jruby/Packer.java b/ext/java/org/msgpack/jruby/Packer.java index 3fd47537..aa10ed14 100644 --- a/ext/java/org/msgpack/jruby/Packer.java +++ b/ext/java/org/msgpack/jruby/Packer.java @@ -64,7 +64,7 @@ public IRubyObject isCompatibilityMode(ThreadContext ctx) { @JRubyMethod(name = "registered_types_internal", visibility = PRIVATE) public IRubyObject registeredTypesInternal(ThreadContext ctx) { - return registry.toRubyHash(ctx); + return registry.toInternalPackerRegistry(ctx); } @JRubyMethod(name = "register_type", required = 2, optional = 1) @@ -98,7 +98,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Blo } RubyClass extClass = (RubyClass) klass; - registry.put(extClass, (int) typeId, proc, arg); + registry.put(extClass, (int) typeId, proc, arg, null, null); return runtime.getNil(); } diff --git a/ext/java/org/msgpack/jruby/Unpacker.java b/ext/java/org/msgpack/jruby/Unpacker.java index 716a8b75..f2a622e7 100644 --- a/ext/java/org/msgpack/jruby/Unpacker.java +++ b/ext/java/org/msgpack/jruby/Unpacker.java @@ -26,7 +26,8 @@ @JRubyClass(name="MessagePack::Unpacker") public class Unpacker extends RubyObject { - public ExtensionRegistry registry; + private final ExtensionRegistry registry; + private IRubyObject stream; private IRubyObject data; private Decoder decoder; @@ -35,7 +36,12 @@ public class Unpacker extends RubyObject { private boolean allowUnknownExt; public Unpacker(Ruby runtime, RubyClass type) { + this(runtime, type, new ExtensionRegistry()); + } + + public Unpacker(Ruby runtime, RubyClass type, ExtensionRegistry registry) { super(runtime, type); + this.registry = registry; this.underflowErrorClass = runtime.getModule("MessagePack").getClass("UnderflowError"); } @@ -45,39 +51,6 @@ public IRubyObject allocate(Ruby runtime, RubyClass klass) { } } - static class ExtensionRegistry { - private Ruby runtime; - public RubyArray[] array; - - public ExtensionRegistry(Ruby runtime) { - this.runtime = runtime; - this.array = new RubyArray[256]; - } - - public ExtensionRegistry dup() { - ExtensionRegistry copy = new ExtensionRegistry(runtime); - copy.array = Arrays.copyOf(array, 256); - return copy; - } - - public void put(RubyClass klass, int typeId, IRubyObject proc, IRubyObject arg) { - IRubyObject k = klass; - if (k == null) { - k = runtime.getNil(); - } - RubyArray e = RubyArray.newArray(runtime, new IRubyObject[] { k, proc, arg }); - array[typeId + 128] = e; - } - - public IRubyObject lookup(int type) { - RubyArray e = array[type + 128]; - if (e == null) { - return null; - } - return e.entry(1); - } - } - @JRubyMethod(name = "initialize", optional = 1, visibility = PRIVATE) public IRubyObject initialize(ThreadContext ctx, IRubyObject[] args) { symbolizeKeys = false; @@ -97,18 +70,12 @@ public IRubyObject initialize(ThreadContext ctx, IRubyObject[] args) { setStream(ctx, args[0]); } } - registry = new ExtensionRegistry(ctx.getRuntime()); return this; } - public void setExtensionRegistry(ExtensionRegistry registry) { - this.registry = registry; - } - public static Unpacker newUnpacker(ThreadContext ctx, ExtensionRegistry extRegistry, IRubyObject[] args) { - Unpacker unpacker = new Unpacker(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Unpacker")); + Unpacker unpacker = new Unpacker(ctx.getRuntime(), ctx.getRuntime().getModule("MessagePack").getClass("Unpacker"), extRegistry); unpacker.initialize(ctx, args); - unpacker.setExtensionRegistry(extRegistry); return unpacker; } @@ -124,13 +91,7 @@ public IRubyObject isAllowUnknownExt(ThreadContext ctx) { @JRubyMethod(name = "registered_types_internal", visibility = PRIVATE) public IRubyObject registeredTypesInternal(ThreadContext ctx) { - RubyHash mapping = RubyHash.newHash(ctx.getRuntime()); - for (int i = 0; i < 256; i++) { - if (registry.array[i] != null) { - mapping.fastASet(RubyFixnum.newFixnum(ctx.getRuntime(), i - 128), registry.array[i].dup()); - } - } - return mapping; + return registry.toInternalUnpackerRegistry(ctx); } @JRubyMethod(name = "register_type", required = 1, optional = 2) @@ -163,7 +124,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Blo throw runtime.newRangeError(String.format("integer %d too big to convert to `signed char'", typeId)); } - registry.put(extClass, (int) typeId, proc, arg); + registry.put(extClass, (int) typeId, null, null, proc, arg); return runtime.getNil(); }