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.

Sign up for GitHub

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

Jump to bottom

[NETBEANS-3693] Create only one instance of javac #1890

Closed
wants to merge 8 commits into from
Closed

[NETBEANS-3693] Create only one instance of javac #1890

wants to merge 8 commits into from

Conversation

Akshay-Gupta-19
Copy link
Contributor

[NETBEANS-3693] Create only one instance of javac for parsing multiple files when refactored

lahodaj
lahodaj requested changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some initial comments. Please note that at least the "org.netbeans.modules.java.source.parsing.JavacParserTest" appears to be failing.

java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java Outdated Show resolved Hide resolved
java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java Outdated Show resolved Hide resolved
java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java
@@ -415,9 +422,28 @@ private void parseImpl(
break;
default:
init (snapshot, task, false);
DiagnosticListener<JavaFileObject> diagnosticListener;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping this could work in such a way that if the processing of the files does not fit into memory, it would fall back to the slow "one javac per file". It is not as good as current nb-javac, but I hope it would work reasonably OK. Would also be good to have some tests for that.

java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java Outdated Show resolved Hide resolved
java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java Outdated Show resolved Hide resolved
@junichi11 junichi11 added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Jan 21, 2020
@jlahoda jlahoda added this to the 12.0 milestone Jan 29, 2020
@Akshay-Gupta-19
Copy link
Contributor Author

Left some initial comments. Please note that at least the "org.netbeans.modules.java.source.parsing.JavacParserTest" appears to be failing.

There is one test case failing - testMultiSourceVanilla
Which has the statement - "assertFalse(Objects.equals(storedJLObject, jlObject));" which I think will not pass when only one instance of javac is created.

@lahodaj
Copy link
Contributor

lahodaj commented Feb 10, 2020

Left some initial comments. Please note that at least the "org.netbeans.modules.java.source.parsing.JavacParserTest" appears to be failing.

There is one test case failing - testMultiSourceVanilla
Which has the statement - "assertFalse(Objects.equals(storedJLObject, jlObject));" which I think will not pass when only one instance of javac is created.

Right - but that means the test needs to be fixed (or removed), correct? I.e. we cannot have failing tests, I assume?

@Akshay-Gupta-19
Copy link
Contributor Author

Left some initial comments. Please note that at least the "org.netbeans.modules.java.source.parsing.JavacParserTest" appears to be failing.

There is one test case failing - testMultiSourceVanilla
Which has the statement - "assertFalse(Objects.equals(storedJLObject, jlObject));" which I think will not pass when only one instance of javac is created.

Right - but that means the test needs to be fixed (or removed), correct? I.e. we cannot have failing tests, I assume?

Yes, will do that.

@lkishalmi
Copy link
Contributor

@jlahoda or @lahodaj as the 12.0 merge window is open, can you review the changes?

@jlahoda
Copy link
Contributor

jlahoda commented Mar 2, 2020

@jlahoda or @lahodaj as the 12.0 merge window is open, can you review the changes?

Currently, without nb-javac, refactoring run file-by-file (one javac instance per file), which increases chances it fits into the memory. This patch changes that to one javac instance for all files in the refactoring - that significantly improves the speed, but lowers the chances the processing will fit into the memory. I still think a middle ground - try to fit all into the memory, and run fast if it fits, but fallback to file-by-file processing if it doesn't - is the best combination. And, unless I am mistaken, this patch does not provide that.

@Akshay-Gupta-19
Copy link
Contributor Author

Akshay-Gupta-19 commented Mar 12, 2020 •
edited

@jlahoda
Does the issue in this PR(Making multiple instances of javac for parsing multiple files) is addressed in this other PR #2013 ??
Or should I keep this one open??

@jlahoda
Copy link
Contributor

jlahoda commented Mar 13, 2020

@jlahoda
Does the issue in this PR(Making multiple instances of javac for parsing multiple files) is addressed in this other PR #2013 ??
Or should I keep this one open??

I don't think there is a significant overlap between these PRs - this PR is (as far as I can tell) speeding up refactoring. #2013 is fixing the refactoring.java tests+adding them into the Travis configuration.

arusinha
arusinha reviewed Apr 28, 2020
View reviewed changes
java/java.source.base/src/org/netbeans/modules/java/source/parsing/JavacParser.java Outdated
@@ -419,15 +425,15 @@ private void parseImpl(
init (snapshot, task, false);
DiagnosticListener<JavaFileObject> diagnosticListener;
JavacTaskImpl javacTask;
if (sequentialParsing == null && ciImpl == null) {
if (sequentialParsing == null && ciImpl == null && this.snapshotSize <= (5<<20)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please declare it as constant like THRESHOLD_SIZE or similar ot that

@neilcsmith-net
Copy link
Member

@ebarboni probably better to stick with what we've been testing with and push this back to 12.1?

@ebarboni ebarboni modified the milestones: 12.0, 12.1 May 6, 2020
@lahodaj
Copy link
Contributor

lahodaj commented Jun 2, 2020

I guess I was hoping the processing would adjust dynamically (i.e. the processing would switch to file-by-file if it would run out of memory).

Anyway - my proposal is to add an option to enable this and push (after master opens), so that we can test in practice.

@lahodaj
Copy link
Contributor

lahodaj commented Jun 2, 2020

By an option, I mean a command line option like -J-Djava.enable.single.javac=true; which can be tested using Boolean.getBoolean("java.enable.single.javac").

@neilcsmith-net
Copy link
Member

What's the status on this one? @lahodaj safe to merge for 12.1?

@lahodaj
Copy link
Contributor

lahodaj commented Jul 21, 2020

Sorry, but the builds say there is some problem with the patch - could you please investigate? Thanks!

@neilcsmith-net
Copy link
Member

At this point in time, think we need to bump this to 12.2.

@neilcsmith-net neilcsmith-net modified the milestones: 12.1, 12.2 Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arusinha arusinha arusinha left review comments

@lahodaj lahodaj lahodaj requested changes

Assignees
No one assigned
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Milestone
12.2
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Akshay-Gupta-19 @lahodaj @lkishalmi @jlahoda @neilcsmith-net @arusinha @junichi11 @ebarboni