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

Add structral comparison barriers #7942

Merged
merged 1 commit into from Dec 9, 2019
Merged

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Dec 2, 2019

Add structral comparison barriers

In #7743 a flag (J9ClassCanSupportFastSubstitutability) is added to each valueType class if it does not contain any floating point primitives or references that might potentially contain an unflattened valuetype or a flattened valuetype that does not contain J9ClassCanSupportFastSubstitutability.

For these classes a structural comparison can be performed. This is essentially performing a memory comparison over the length of the instances, if they are equivalent the result is true, otherwise false. Since it will only contain scalar primitives and (non value) references, equality can be determined by simply performing a memory comparison. Unlike with floating point types or non-flattened valuetypes where determining equality is more complicated.

A barrier helper is required as these instances may contain references. In modes where no read barriers are required memcmp can be used.

Signed-off-by: Tobi Ajila atobia@ca.ibm.com

@amicic
Copy link
Contributor

amicic commented Dec 2, 2019

I'd avoid using Flattened in the name. Ideally, it should work even if objects are not flattened.

@tajila
Copy link
Contributor Author

tajila commented Dec 2, 2019

I'd avoid using Flattened in the name. Ideally, it should work even if objects are not flattened.

If it is not flattened we need to do a different more complicated type of comparison similar to comparing if two tree top nodes are the same. We have a separate implementation for this in the interpreter that makes use of the existing barrier APIs and a some of the VM fieldIterator APIs. The flattened case is special because we currently have no barrier APIs that perform the equivalent of memcmp.

In short, we would only use this API if the object was flattened.

@tajila
Copy link
Contributor Author

tajila commented Dec 2, 2019

That being said, I have no objection to changing the name

@amicic
Copy link
Contributor

amicic commented Dec 4, 2019

Looks good to me.

@tajila
Copy link
Contributor Author

tajila commented Dec 5, 2019

@amicic The required changes have been made

@tajila tajila changed the title WIP: Add structral comparison barriers Add structral comparison barriers Dec 5, 2019
@amicic
Copy link
Contributor

amicic commented Dec 5, 2019

Jenkins test sanity all jdk11

@tajila
Copy link
Contributor Author

tajila commented Dec 5, 2019

@amicic THe failures are all git checkout failures

Checking out Revision b4ce7d840de92cf2a8a008f976e62b93fd73260e (refs/remotes/origin/pr/7942/merge)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f b4ce7d840de92cf2a8a008f976e62b93fd73260e
ERROR: Timeout after 10 minutes
ERROR: Checkout failed
hudson.plugins.git.GitException: Command "git checkout -f b4ce7d840de92cf2a8a008f976e62b93fd73260e" returned status code 143:
stdout: 
stderr: 
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2172)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$1000(CliGitAPIImpl.java:78)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$9.execute(CliGitAPIImpl.java:2471)
Caused: hudson.plugins.git.GitException: Could not checkout b4ce7d840de92cf2a8a008f976e62b93fd73260e
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$9.execute(CliGitAPIImpl.java:2495)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1242)
	at org.jenkinsci.plugins.workflow.steps.scm.SCMStep.checkout(SCMStep.java:124)
	at org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition.create(CpsScmFlowDefinition.java:149)
	at org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition.create(CpsScmFlowDefinition.java:67)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:299)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:429)

@dmitripivkine
Copy link
Contributor

@tajila Would you please resolve merging conflicts?

In eclipse-openj9#7743 a flag (J9ClassCanSupportFastSubstitutability) is added to each
valueType class if it does not contain any floating point primitives or
references that might potentially contain an unflattened valuetype or a
flattened valuetype that does not contain
J9ClassCanSupportFastSubstitutability.

For these classes a structural comparison can be performed. This is
essentially performing a memory comparison over the length of the
instances, if they are equivalent the result is true, otherwise false.
Since it will only contain scalar primitives and (non value) references,
equality can be determined by simply performing a memory comparison.
Unlike with floating point types or non-flattened valuetypes where
determining equality is more complicated.

A barrier helper is required as these instances may contain references.
In modes where no read barriers are required memcmp can be used.

Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>
@dmitripivkine
Copy link
Contributor

Jenkins test sanity all jdk11

@tajila
Copy link
Contributor Author

tajila commented Dec 6, 2019

@dmitripivkine @amicic The only failure is a jenkins failure

Setting status of 131768462c4271461ddd46988a9231ca262f6e26 to FAILURE with url https://ci.eclipse.org/openj9/job/PullRequest-OpenJ9/2133/ and message: ' '
Using context: Pull Request - OpenJ9
hudson.remoting.ProxyException: groovy.lang.MissingMethodException: No signature of method: java.util.ArrayList.keySet() is applicable for argument types: () values: []
Possible solutions: toSet(), toSet(), set(int, java.lang.Object), set(int, java.lang.Object), get(int), get(int)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onMethodCall(SandboxInterceptor.java:153)
	at org.kohsuke.groovy.sandbox.impl.Checker$1.call(Checker.java:158)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedCall(Checker.java:162)
	at com.cloudbees.groovy.cps.sandbox.SandboxInvoker.methodCall(SandboxInvoker.java:17)
	at Script3.get_test_target_names(Script3.groovy:477)
	at Script3.get_downstream_job_names(Script3.groovy:582)
	at WorkflowScript.get_summary_table(WorkflowScript:511)
	at com.cloudbees.groovy.cps.CpsDefaultGroovyMethods.each(CpsDefaultGroovyMethods:2030)
	at com.cloudbees.groovy.cps.CpsDefaultGroovyMethods.each(CpsDefaultGroovyMethods:2015)
	at com.cloudbees.groovy.cps.CpsDefaultGroovyMethods.each(CpsDefaultGroovyMethods:2056)
	at WorkflowScript.get_summary_table(WorkflowScript:508)
	at WorkflowScript.run(WorkflowScript:357)
	at ___cps.transform___(Native Method)
	at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:84)

@pshipton
Copy link
Member

pshipton commented Dec 6, 2019

The "Jenkins failure" means that no testing ran, only the compiles. It should be fixed now, retrying.

Jenkins test sanity all jdk11

@pshipton
Copy link
Member

pshipton commented Dec 6, 2019

I forgot, I think the commit needs to be rebased in order to fix the PR testing.

@pshipton
Copy link
Member

pshipton commented Dec 6, 2019

Another PR worked without being rebased. I had attempted to stop the earlier build, so I'll run it again.

Jenkins test sanity all jdk11

@tajila
Copy link
Contributor Author

tajila commented Dec 9, 2019

@amicic @dmitripivkine All checks have passed

@amicic amicic merged commit 065e210 into eclipse-openj9:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants