[jruby] cleanup/refactor extension code #1934

kares · 2019-10-16T14:26:54Z

There's some anti-patterns regarding JRuby ext / Java to revisit (still an ongoing effort).
The Java ext part is confusing at times - initializing parts twice (e.g. XmlDocument's node).

Relevant changes :

avoid ns-cache being double backed by a map + another key list (use just map backing)
also introduce a cache-key as using arrays was weird - potentially leaky on some scenarios
do not construct ext wrapper objects when internally XML API objects are sufficient
cleaner and more explicit way of initializing (document) internals for XmlNodeSets
align with JRuby's toJava convention (will also now work for any Node not just Document)
review some of the internals and remove dead-code
do not silently swallow potentially fatal exception from backend

Nokogiri is a bit heavy on memory on times - there's a few attempts to reduce allocations here.

NOTE: includes #1930 (to avoid potential conflict resolution) should be without conflicts after its merged.

This also could use some raw performance numbers.

codeclimate · 2019-10-16T14:29:26Z

ext/java/nokogiri/XmlNode.java

        }

        try{
            int res = node.compareDocumentPosition(otherNode);
            if ((res & FIRST_PRECEDES_SECOND) == FIRST_PRECEDES_SECOND) {
-                return context.getRuntime().newFixnum(-1);
+                return context.runtime.newFixnum(-1);


Avoid too many return statements within this method.

codeclimate · 2019-10-16T14:29:26Z

ext/java/nokogiri/internals/NokogiriXPathFunction.java

@@ -116,7 +117,7 @@ private static Object fromRubyToObject(final Ruby runtime, IRubyObject obj) {
        }
        if (obj instanceof XmlNodeSet) return obj;
        if (obj instanceof RubyArray) {
-            return XmlNodeSet.newXmlNodeSet(runtime.getCurrentContext(), ((RubyArray) obj).toJavaArray());
+            return XmlNodeSet.newNodeSet(runtime, ((RubyArray) obj).toJavaArray());


Avoid too many return statements within this method.

codeclimate · 2019-10-16T14:29:26Z

ext/java/nokogiri/XmlNode.java

        } catch (Exception ex) {
-            return context.getRuntime().newFixnum(-2);
+            return context.runtime.newFixnum(-2);


Avoid too many return statements within this method.

codeclimate · 2019-10-16T14:29:26Z

ext/java/nokogiri/XmlNode.java

            }

-            return context.getRuntime().newFixnum(-2);
+            return context.runtime.newFixnum(-2);


Avoid too many return statements within this method.

codeclimate · 2019-10-16T14:29:27Z

ext/java/nokogiri/internals/NokogiriNamespaceCache.java

+            this.href = href;
+        }
+
+        @Override


Method equals has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2019-10-16T14:29:29Z

Code Climate has analyzed commit 604a0db and detected 37 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	13
Duplication	24

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 46.3%.

View more on Code Climate.

... since it does (unwanted) auto-conversion toJava

every time a new XML document is being created, since it causes class-path scanning (considerable slow-downs esp. in multi-threaded envs)

interestingly the String[] key was pretty much a leak not perfect but at least more performing than before

potentially avoid double initialization of document node also make document cache explicit - no null side effects

and Document.wrap impls - override toJava on XmlNode that way any node type can unwrap its Java 'backend'

@document

just loop node-list children directly internally make it explicit that node-set needs to initialize (@document)

+ cleanup some of the internal helpers

codeclimate · 2019-10-16T18:42:22Z

ext/java/nokogiri/XmlNode.java

+        visitor.leave(node);
+    }
+
+    void acceptChildren(ThreadContext context, IRubyObject[] nodes, SaveContextVisitor visitor) {


Method acceptChildren has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2019-10-16T18:42:23Z

ext/java/nokogiri/XmlNode.java

-    private void addElements(Node n, List<Node> nodes, boolean isFirstOnly) {
-        NodeList children = n.getChildNodes();
-        if (children.getLength() == 0) return;
+    private static List<Node> getElements(Node node, final boolean firstOnly) {


Method getElements has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2019-10-16T18:42:23Z

ext/java/nokogiri/HtmlDocument.java

-    private void stabilzeAttrValue(Node node) {
-        if (node == null) return;
+
+    private static void stabilizeAttrs(Node node) {


Method stabilizeAttrs has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2019-10-16T18:42:23Z

ext/java/nokogiri/XmlDocument.java

        clearXpathContext(getNode());
        return this;
    }

-    private void removeNamespceRecursively(ThreadContext context, XmlNode xmlNode) {
+    private void removeNamespaceRecursively(XmlNode xmlNode) {


Method removeNamespaceRecursively has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2019-10-16T18:42:23Z

ext/java/nokogiri/XmlNamespace.java

-        String prefixValue = prefix.isNil() ? "" : (String) prefix.toJava(String.class);
-        String hrefValue = (String) href.toJava(String.class);
-        Ruby runtime = prefix.getRuntime();
+    static XmlNamespace createImpl(Node owner, IRubyObject prefix, String prefixStr, IRubyObject href, String hrefStr) {


Method createImpl has 5 arguments (exceeds 4 allowed). Consider refactoring.

flavorjones · 2019-11-26T13:25:05Z

@kares Thanks so much for all the work that went into this patch set. Very much appreciated!

kares added 5 commits October 7, 2019 15:34

let JRuby do the char -> byte encoding - do not hard-core UTF-8

8758649

review non-null rubyToString + simplify internals

d9af178

no pre JRuby 1.7 code needed these days

6b3902f

cleanup and revisit JRuby's HtmlSaxParserContext impl

aa9b8c4

JRuby ext house-keeping cleanup/refactor some of the internals

c2ab319

codeclimate bot reviewed Oct 16, 2019

View reviewed changes

kares added 18 commits October 16, 2019 18:37

[refactor] simplify xml name-space (cache) handling

3dd0d83

[refactor] do not use Iterator over RubyArray

32c6ca8

... since it does (unwanted) auto-conversion toJava

[refactor] invalid node handling (avoid NPE)

ad0c735

[refactor] node (cached) name - always a RubyString

2220195

[fix] avoid doing document factory finder lookup

827d7ba

every time a new XML document is being created, since it causes class-path scanning (considerable slow-downs esp. in multi-threaded envs)

[refactor] use toArray snapshot instead of loop

4aca36c

[refactor] share XmlDocument init part

3c55b31

[refactor] read parent once, fix str == "" check

e855d8f

[refactor] ns-cache using a namespace prefix+href key

c0058b3

interestingly the String[] key was pretty much a leak not perfect but at least more performing than before

[refactor] simplify - doc/node seting internals

f9bc4f4

potentially avoid double initialization of document node also make document cache explicit - no null side effects

[fix] static pattern instance will do just fine

ab18a4f

[refactor] remove (dead) un-used methods

a9be97f

[refactor] simplify JRuby's Document#to_java

88fcb20

and Document.wrap impls - override toJava on XmlNode that way any node type can unwrap its Java 'backend'

[refactor] more cleanup (and dry-ing) of ext internals

f41f618

[refactor] dry-out JRuby's DocumentFragment#add_child impl

6129285

[refactor] actually add_child impl is dead-code

77df21b

[refactor] node-set handling - avoid when unnecessary

84a554f

just loop node-list children directly internally make it explicit that node-set needs to initialize (@document)

[refactor] reduce noise while retrieving node elements

604a0db

+ cleanup some of the internal helpers

kares force-pushed the jruby-review-8_DETACHED-merge-back branch from 40bfade to 604a0db Compare October 16, 2019 18:40

codeclimate bot reviewed Oct 16, 2019

View reviewed changes

flavorjones approved these changes Nov 26, 2019

View reviewed changes

flavorjones merged commit 8a1c981 into sparklemotion:master Nov 26, 2019

flavorjones mentioned this pull request Nov 26, 2019

[fix] avoid doing (Java) document factory finder lookup #1930

Closed

flavorjones added this to the v1.11.0 milestone Nov 26, 2019

flavorjones mentioned this pull request Aug 18, 2021

[jruby] HTML::DocumentFragment.parse throws a NPE when namespaced attributes are used #1836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jruby] cleanup/refactor extension code #1934

[jruby] cleanup/refactor extension code #1934

kares commented Oct 16, 2019 •

edited

Loading

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot commented Oct 16, 2019 •

edited

Loading

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

codeclimate bot Oct 16, 2019

flavorjones commented Nov 26, 2019

[jruby] cleanup/refactor extension code #1934

[jruby] cleanup/refactor extension code #1934

Conversation

kares commented Oct 16, 2019 • edited Loading

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot commented Oct 16, 2019 • edited Loading

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

codeclimate bot Oct 16, 2019

Choose a reason for hiding this comment

flavorjones commented Nov 26, 2019

kares commented Oct 16, 2019 •

edited

Loading

codeclimate bot commented Oct 16, 2019 •

edited

Loading