Skip to content
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.

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

Use com.jcraft.jzlib.JZlib instead of hacking the internals of java.util.zip.CRC32 #5088

Closed
wants to merge 2 commits into from

Conversation

@jmiettinen
Copy link
Contributor

@jmiettinen jmiettinen commented Mar 16, 2018

The code the seems to be a remnant of the time before JZLib and uses internals of java.util.zip.CRC32 when same thing could be achieved through JZLib.

This can cause a performance regression as methods in CRC32 use intrinsics, but I would not expect that to be major issue as running a lot of CRC32 does not sound like the ideal use case for JRuby.

This should remove one invasive access warning from #4834

@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Mar 16, 2018

Now that I got the results deciphered, it seems that there's actually a problem there in CRC32.gf2_matrix_times.

I'll look into this.

@headius
Copy link
Member

@headius headius commented Mar 27, 2018

@jmiettinen Any luck?

@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Mar 28, 2018

I'll check this out during easter holidays this weekend.

@headius
Copy link
Member

@headius headius commented Mar 28, 2018

Not a super time-critical item so don't sweat it 😀 Looking at 9.2 release in about a month.

@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Apr 3, 2018

Is there a way to run the build again as I don't see my changes stalling the build totally?

@headius
Copy link
Member

@headius headius commented Apr 3, 2018

I restarted the whole thing. But when did you pull this from master? We haven't tagged off or completed the remaining 2.5 failures, so that could be the source of much of this. The hangs are unfortunately elusive...something concurrency-related in MRI's tests that we stumble over.

@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Apr 4, 2018

I probably should've started from some known-to-work tag instead of just branching from whatever happened to be the master at that time (334a7f0).

Should I rebase this on some known-to-work-tag and force-push, or what's the preferred way of working here?

@headius
Copy link
Member

@headius headius commented Apr 12, 2018

@jmiettinen Rebasing against current master is probably your best bet. Do that and repush and we'll confirm that the failures match expected.

I'm working today to try to get master properly green, also.

jmiettinen added 2 commits Mar 14, 2018
…at's the way to simulate 32-bit unsigned ints in the JVM
@jmiettinen jmiettinen force-pushed the jmiettinen:jarkko-4834 branch from 8e6228c to bb53bd9 Apr 20, 2018
@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Apr 20, 2018

I rebased this on master that had a passing Travis build.

Now I get these errors:

     [exec]   test_symlinked_jar:					SyntaxError: /home/travis/build/jruby/jruby/test/jruby/jarwithoutextension:1: Invalid char `\3' ('�') in expression

Any ideas about these? I can't really see any connection between that I've done and reading jars. @headius

@headius
Copy link
Member

@headius headius commented Apr 20, 2018

That has had some failing builds due to another PR that tried to fix some aspects of symlink traversal during load. It is probably not you. What reg did you branch from? I'm fine merging this.

@jmiettinen
Copy link
Contributor Author

@jmiettinen jmiettinen commented Apr 23, 2018

I rebased this on e7bf9d1 which has a passing build: https://travis-ci.org/jruby/jruby/builds/367136555. Also, there does not seem to be any connection between CRC32 and the JAR-tests.

That's why I am a bit confused here.

@kares
Copy link
Member

@kares kares commented Apr 25, 2018

due some local issues with Java 8 I'm now running under 9 and hit this 3 liner warning.
considering merging as all of the failures seem non related -> have seen them on master

@kares
Copy link
Member

@kares kares commented Apr 26, 2018

vefiried and landed manually as: f1f2c72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.