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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ji] package (internal) refactoring #3754

Merged
merged 21 commits into from Mar 29, 2016

Conversation

Projects
None yet
3 participants
@kares
Member

kares commented Mar 22, 2016

mostly some cleanup around JavaPackageTemplate which is replaced by a native JavaPackage

... allows for pieces to stay at one place - and avoid 'hacks' such as setting @package_name.

compatibility should be fine, I see too (minor) "breaking" changes :

  • java.lang.class will return Java::JavaPackage instead of Module (believe this is for the better)

  • was thinking about exposing more methods (with the BlankSlateWrapper hack) on packages, but for now I hold this back (left them commented) :

    • singleton_class since class and name work as expected and its very low risk to collide
    • throw, catch as those are Java keywords packages couldn't use those names

    SOME (FUTURE) IDEAS :

  • we might allow packages to behave like a Ruby object (with a -Xji switch) - no method filtering

  • for conflicting package names - method access wouldn't work but constant access still might :

e.g. org.jruby.test.name.MyClass 馃敶 (due name) but org::jruby::test::name::MyClass 馃崗 but maybe its not such a good idea - let me know what are your thoughts on that. currently users need to resort to Java::OrgJrubyTestName::... or java_import 'org.jruby.test.name' in such cases.

numbers show performance is around the same if not better on some runs :

9.0.5.

Rehearsal ---------------------------------------------------------------------------------
Java::java::lang [5000000x]                     2.680000   1.000000   3.680000 (  2.475752)
org.ietf.jgss [5000000x]                        1.590000   0.040000   1.630000 (  1.327823)
org.xml.sax.helpers.DefaultHandler [5000000x]   1.560000   0.060000   1.620000 (  1.479342)
Java.pkg0...pkg9 5000000x]                      0.710000   0.000000   0.710000 (  0.590146)
------------------------------------------------------------------------ total: 7.640000sec

                                                    user     system      total        real
Java::java::lang [5000000x]                     1.320000   0.000000   1.320000 (  1.303038)
org.ietf.jgss [5000000x]                        1.380000   0.020000   1.400000 (  1.379260)
org.xml.sax.helpers.DefaultHandler [5000000x]   1.560000   0.040000   1.600000 (  1.531264)
Java.pkg0...pkg9 5000000x]                      0.500000   0.000000   0.500000 (  0.495313)

9.1.0

Rehearsal ---------------------------------------------------------------------------------
Java::java::lang [5000000x]                     2.440000   0.440000   2.880000 (  1.788888)
org.ietf.jgss [5000000x]                        1.490000   0.300000   1.790000 (  1.462861)
org.xml.sax.helpers.DefaultHandler [5000000x]   1.270000   0.330000   1.600000 (  1.497588)
Java.pkg0...pkg9 5000000x]                      0.580000   0.000000   0.580000 (  0.426573)
------------------------------------------------------------------------ total: 6.850000sec

                                                    user     system      total        real
Java::java::lang [5000000x]                     1.100000   0.110000   1.210000 (  1.175431)
org.ietf.jgss [5000000x]                        1.310000   0.100000   1.410000 (  1.224360)
org.xml.sax.helpers.DefaultHandler [5000000x]   1.130000   0.170000   1.300000 (  1.272022)
Java.pkg0...pkg9 5000000x]                      0.380000   0.000000   0.380000 (  0.352696)

@kares kares added this to the JRuby 9.1.0.0 milestone Mar 22, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 22, 2016

Member

Since JavaPackage extends Module I do not see this change as one which could possibly break anything (knock on wood).

I am interested in an enumerated list of package names which would be invalid before and after the change. At a minimum, having a list like this would be great for documentation.

module JavaPackageModuleTemplate disappears from Java since it is private but you also remove the Ruby version as well. I wonder if anyone in the wild uses this (perhaps alias JavaPackage to this just in case?).

I know you did not do this based on diff but there are many method calls which are prefixed with 'this.'. If you want to kill those that would be nice.

You changed visibilty of method_missing and const_missing to private which I think is correct but I am just mentioning it in case someone knows why non-private would still be needed or could cause problems.

All-in-all I have no issues with the PR itself my only question would be to challenge which behavior could have possibly changed which will generate an issue report.

Member

enebo commented Mar 22, 2016

Since JavaPackage extends Module I do not see this change as one which could possibly break anything (knock on wood).

I am interested in an enumerated list of package names which would be invalid before and after the change. At a minimum, having a list like this would be great for documentation.

module JavaPackageModuleTemplate disappears from Java since it is private but you also remove the Ruby version as well. I wonder if anyone in the wild uses this (perhaps alias JavaPackage to this just in case?).

I know you did not do this based on diff but there are many method calls which are prefixed with 'this.'. If you want to kill those that would be nice.

You changed visibilty of method_missing and const_missing to private which I think is correct but I am just mentioning it in case someone knows why non-private would still be needed or could cause problems.

All-in-all I have no issues with the PR itself my only question would be to challenge which behavior could have possibly changed which will generate an issue report.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 22, 2016

Member

I am interested in an enumerated list of package names which would be invalid before and after the change. At a minimum, having a list like this would be great for documentation.

as noted in the desc it stayed the same (for now) except for :
- singleton_class since class and name work as expected and its very low risk to collide
- throw, catch as those are Java keywords packages couldn't use those names

so really only singleton_class would be prohibited after the merge with package method access

module JavaPackageModuleTemplate disappears from Java since it is private but you also remove the Ruby version as well. I wonder if anyone in the wild uses this (perhaps alias JavaPackage to this just in case?).

will look into some github usage if anyone has done something of a kind. if not I'll leave it dead as is.

I know you did not do this based on diff but there are many method calls which are prefixed with 'this.'. If you want to kill those that would be nice.

NP - will do, thanks.

You changed visibilty of method_missing and const_missing to private which I think is correct but I am just mentioning it in case someone knows why non-private would still be needed or could cause problems.

yeah I noticed that and fixed in a follow-up commit: 744bf4e ... so this should be the same as it was.

Member

kares commented Mar 22, 2016

I am interested in an enumerated list of package names which would be invalid before and after the change. At a minimum, having a list like this would be great for documentation.

as noted in the desc it stayed the same (for now) except for :
- singleton_class since class and name work as expected and its very low risk to collide
- throw, catch as those are Java keywords packages couldn't use those names

so really only singleton_class would be prohibited after the merge with package method access

module JavaPackageModuleTemplate disappears from Java since it is private but you also remove the Ruby version as well. I wonder if anyone in the wild uses this (perhaps alias JavaPackage to this just in case?).

will look into some github usage if anyone has done something of a kind. if not I'll leave it dead as is.

I know you did not do this based on diff but there are many method calls which are prefixed with 'this.'. If you want to kill those that would be nice.

NP - will do, thanks.

You changed visibilty of method_missing and const_missing to private which I think is correct but I am just mentioning it in case someone knows why non-private would still be needed or could cause problems.

yeah I noticed that and fixed in a follow-up commit: 744bf4e ... so this should be the same as it was.

kares added some commits Aug 12, 2015

introduce (native) JavaPackage replacement for JavaPackageModuleTemplate
with makeMetaClass - previous package module meta-class attaching
... should no longer be necessary
handle more Kernel methods in Java packages "right" (including name)
- `pkg.name` has been working since 1.7.22/9.0.1.0 (#2468)
- handling :object_id as it is quite surprising to not have
- can handle :throw/:catch since they're not valid package names
- commented-out methods that would be good to have as well
[test] how packages behave when a method collides (e.g. on `pkg.name`)
and assert that `pkg.object_id` works as expected!
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 22, 2016

Member

people actually seems to use JavaPackageModuleTemplate for some "hacks" ... will need to understand what they're doing and likely setup a deprecated alias.

addressed the rest and squashed some of the commits for more clarity.

UPDATE: so those are all copies of old JRuby's stdlib files - no usage found really.

Member

kares commented Mar 22, 2016

people actually seems to use JavaPackageModuleTemplate for some "hacks" ... will need to understand what they're doing and likely setup a deprecated alias.

addressed the rest and squashed some of the commits for more clarity.

UPDATE: so those are all copies of old JRuby's stdlib files - no usage found really.

kares added some commits Mar 23, 2016

[ji] keep (deprecated) JavaPackageModuleTemplate constant for compati鈥
鈥ility

... its probably fine to remove - but just in case
[ji] packages shall try to handle respond_to? and respond_to_missing?鈥
鈥 right

... also restored singleton method hooks for compatibility with previous versions

@kares kares merged commit ae94c39 into master Mar 29, 2016

0 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 6, 2016

Member

for the record - dealing with the introduced proxy class initialization regression: f1280b1

Member

kares commented Apr 6, 2016

for the record - dealing with the introduced proxy class initialization regression: f1280b1

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 6, 2016

Member

@kares can this get cherry-picked to 1.7 also?

Member

enebo commented Apr 6, 2016

@kares can this get cherry-picked to 1.7 also?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 6, 2016

Member

@enebo these (package) changes are not there -- thus makes no sense, or does it 馃樃 ?
jruby-1_7 shouldn't have had issues with the proxy initialization if so its due something else ...

Member

kares commented Apr 6, 2016

@enebo these (package) changes are not there -- thus makes no sense, or does it 馃樃 ?
jruby-1_7 shouldn't have had issues with the proxy initialization if so its due something else ...

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 6, 2016

Member

@kares oh...yeah I don't know why it is happening then. I figured it was the same issue. Weird that both branches have the same failing test.

Member

enebo commented Apr 6, 2016

@kares oh...yeah I don't know why it is happening then. I figured it was the same issue. Weird that both branches have the same failing test.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 12, 2016

Member

// cc @Lan5432 - sorry forgot, its been merged already but could be interesting to learn some JI internals

Member

kares commented Apr 12, 2016

// cc @Lan5432 - sorry forgot, its been merged already but could be interesting to learn some JI internals

@Lan5432

This comment has been minimized.

Show comment
Hide comment
@Lan5432

Lan5432 Apr 12, 2016

Contributor

Yeah, I will take a look into the commits and the discussion. Thanks anyways, I know the 9.1 release is an important task

Contributor

Lan5432 commented Apr 12, 2016

Yeah, I will take a look into the commits and the discussion. Thanks anyways, I know the 9.1 release is an important task

@kares kares deleted the ji-package-refactor branch Sep 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment