jBallerina Compiler Backend Rewrite #21435

Kishanthan · 2020-03-03T10:33:44Z

Purpose

$subject.

Approach

Describe how you are implementing the solutions along with the design details.

Samples

Provide high-level details about the samples related to this feature.

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

Migrate JvmTerminatorGen and fix compilation errors

Remove BIRVarRef class and use BIROperand instead

…rm/ballerina-lang into jbal-be-rewrite

Migrate JvmPackageGen and fix compilation errors

Migrate JvmObservabilityGen, JvmLabelGen and fix compilation errors

Migrate JvmMethodGen to java

Migrate interop gen related classes

Fix runtime bugs to get migrated backend working for empty function

Fix InteropMethodGen compilation errors and conflicts

…rm/ballerina-lang into jbal-be-rewrite

Clean JInteropFieldValidator and Use BackendDriver for test source compilation

…rm/ballerina-lang into jbal-be-rewrite

Change the class loader in tests to system class loader

riyafa · 2020-03-03T13:33:51Z

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BackendDriver.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.wso2.ballerinalang.compiler.bir;


Shall we add a new line in all places?

Suggested change

package org.wso2.ballerinalang.compiler.bir;

package org.wso2.ballerinalang.compiler.bir;

riyafa · 2020-03-04T03:51:47Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+
+    public static void generateBToJCheckCast(MethodVisitor mv, BType sourceType, JType targetType) {
+
+        if (targetType.jTag == JTypeTags.JBYTE) {


Why don't we use a switch statement here?

riyafa · 2020-03-04T03:54:56Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+
+        if (sourceType.tag == TypeTags.BYTE) {
+            // do nothing
+        } else if (sourceType.tag == TypeTags.INT) {


Shall we use switch statement?

riyafa · 2020-03-04T03:55:32Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+    private static void generateCheckCastBToJString(MethodVisitor mv, BType sourceType) {
+
+        if (sourceType.tag == TypeTags.STRING) {
+            mv.visitMethodInsn(INVOKESTATIC, STRING_UTILS, "fromString",


Shall we use a constant for hardcoded method name?

riyafa · 2020-03-04T03:56:11Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+
+    private static void generateCheckCastBToJChar(MethodVisitor mv, BType sourceType) {
+
+        if (sourceType.tag == TypeTags.BYTE) {


Shall we use switch statements in all places instead of if-else-if statements?

riyafa · 2020-03-04T03:56:55Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+            mv.visitInsn(D2I);
+            mv.visitInsn(I2S);
+        } else if (sourceType.tag == TypeTags.HANDLE) {
+            mv.visitMethodInsn(INVOKEVIRTUAL, HANDLE_VALUE, "getValue", "()Ljava/lang/Object;", false);


Shall we use constants for the hardcoded values in all places?

why?, I personally find it easier to read when I can see the value.

If we change the name of the method in the future we would need to change only a constant instead of grepping to change all files which can create mistakes. It is a best practice to define method names as constants

Code is read more often than changed, so it better if its easier to read, not easier to modify. In my opinion you need to question the this kind of best practices than blindly trust them.

riyafa · 2020-03-04T05:15:48Z

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java

@@ -460,6 +461,7 @@ public void visit(BLangFunction astFunc) {
        // Special %0 location for storing return values
        birFunc.returnVariable = new BIRVariableDcl(astFunc.pos, astFunc.symbol.type.getReturnType(),
                this.env.nextLocalVarId(names), VarScope.FUNCTION, VarKind.RETURN, null);
+        birFunc.localVars.add(0, birFunc.returnVariable);


This add(0, is used in multiple places. A new reader might not know what 0 signifies. If we define a constant for 0 it would add clarity.

…write

riyafa · 2020-03-04T05:27:24Z

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmCastGen.java

+                //    return;
+                //} else if (sourceType is bir:BRecordType && (targetType is bir:BMapType && targetType.constraint
+                // is bir:BTypeAny)) {
+                //    // do nothing


Shall we remove commented out code in all places before merging?

riyafa · 2020-03-04T05:40:37Z

...lerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmInstructionGen.java

+
+        //    # Generate adding a new value to an array
+//    #
+//    # + inst - array store instruction


Shall we fix these strange comments?

riyafa · 2020-03-04T05:43:40Z

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmMethodGen.java

+                    if (!isBuiltinModule) {
+                        generateObjectArgs(mv, paramIndex);
+                        paramIndex += 1;
+


We really need switch statements.

anupama-pathirage · 2020-03-04T06:03:53Z

@Kishanthan This pr topic does not adhere to our guidelines. Refer https://github.com/ballerina-platform/ballerina-lang/blob/master/CONTRIBUTING.md for details.
We follow https://chris.beams.io/posts/git-commit/ for commit messages and PR topics.
Pls change the PR topic accordingly.
Basically a properly formed Git commit subject line/PR topic should always be able to complete the following sentence:
If applied, this commit/PR will your subject line here

riyafa · 2020-03-04T06:06:57Z

Shall we track disabled tests by creating issues? Otherwise we may miss on fixing them.

see travis-ci/travis-ci#8667

manuranga · 2020-03-04T08:47:00Z

Shall we track disabled tests by creating issues? Otherwise we may miss on fixing them.

+1

reason we haven't cleaned up this code is to make it easy to port all the changes you (and Vinod) has been doing to this branch. we are planing to do a major refactoring after that.

Disable balo path test

codecov-io · 2020-03-04T10:41:12Z

Codecov Report

Merging #21435 into master will increase coverage by 4.5%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #21435     +/-   ##
=========================================
+ Coverage   14.67%   19.17%   +4.5%     
=========================================
  Files          51       56      +5     
  Lines        1404     1528    +124     
  Branches      215      233     +18     
=========================================
+ Hits          206      293     +87     
- Misses       1182     1219     +37     
  Partials       16       16

Impacted Files	Coverage Δ
tool-plugins/vscode/src/extension.ts	`0% <0%> (ø)`	⬆️
composer/packages/tracing/src/ToolBar.tsx	`71.05% <0%> (ø)`
composer/packages/tracing/src/TraceList.tsx	`94.44% <0%> (ø)`
composer/packages/tracing/src/DetailView.tsx	`72.72% <0%> (ø)`
composer/packages/tracing/src/TraceLogs.tsx	`70.37% <0%> (ø)`
composer/packages/tracing/src/index.ts	`0% <0%> (ø)`
tool-plugins/vscode/src/core/extension.ts	`28.37% <0%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc5ec12...a68378a. Read the comment docs.

riyafa · 2020-03-05T02:01:18Z

Shall we track disabled tests by creating issues? Otherwise we may miss on fixing them.

+1

reason we haven't cleaned up this code is to make it easy to port all the changes you (and Vinod) has been doing to this branch. we are planing to do a major refactoring after that.

The reason why that "We are going to refactor later" is normally a lie in the software industry:

It normally means the authors don't want to spend time on it now and probably will find more important tasks to work on later so refactoring will never be done.
Every one can't have refactoring code as a task on their sprint so the original authors will pass maintenance to some one else who don't understand the code so that they can move onto something interesting to do. And these new people don't know the head or tail to fix it.
The broken window issue. Once the code is degraded future maintainers are not going to fix it, they are going to follow the general pattern of writing bad code.
Refactoring code is a time consuming task. And probably will take more time than writing the original "something that works" code so the original authors can't account for this time when they have already informed the higher management that they have already completed the task.

Once the original authors leave, the code will be no longer salvageable and the complexities will remain forever or someone else will try to rewrite and again the same loop will repeat.

Read The Total Cost of Owning a Mess in Chapter 1 of Clean code book: https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882
Please note I am talking in the general sense and not very specific to this PR

Kishanthan and others added 30 commits February 12, 2020 11:26

Migrate JvmTerminatorGen and fix compilation errors

6088c3e

Use correct BFutureType

21d338f

Merge pull request #21007 from Kishanthan/jbal-be-rewrite

d3c9283

Migrate JvmTerminatorGen and fix compilation errors

Remove BIRVarRef class and use BIROperand instead

42d2350

Merge pull request #21014 from Kishanthan/jbal-be-rewrite

c77a183

Remove BIRVarRef class and use BIROperand instead

Migrate JvmInstructionGen to java

18d039f

Migrate JvmPackageGen and fix compilation errors

f4340c2

Merge branch 'jbal-be-rewrite' of https://github.com/ballerina-platfo…

f09c688

…rm/ballerina-lang into jbal-be-rewrite

Merge with upstream

95e2bcc

Fix compilation errors with addJUnboxInsn method

5e36d2f

Merge pull request #21022 from Kishanthan/jbal-be-rewrite

bf76bf3

Migrate JvmPackageGen and fix compilation errors

Migrate JvmObservabilityGen, JvmLabelGen and fix compilation errors

059a5d2

Update java docs

1695fc3

Merge pull request #21023 from Kishanthan/jbal-be-rewrite

2a21af0

Migrate JvmObservabilityGen, JvmLabelGen and fix compilation errors

Migrate JvmMethodGen to java

05af260

Migrate interop gen related classes

984d668

Merge pull request #21077 from manuranga/jbal-be-rewrite

2ba6e84

Migrate JvmMethodGen to java

Fix merge conlficts and compilation errors

adb6a6a

Merge pull request #21087 from Kishanthan/jbal-be-rewrite

d787202

Migrate interop gen related classes

Fix runtime bugs to get migrated backend working for empty function

f0e863c

Fix genJFieldForInteropField compilation errors

4d2a10d

Merge pull request #21115 from manuranga/jbal-be-rewrite

51b26f0

Fix runtime bugs to get migrated backend working for empty function

Fix InteropMethodGen compilation errors and conflicts with upstream

057c883

Merge pull request #21121 from Kishanthan/jbal-be-rewrite

ea3b4a3

Fix InteropMethodGen compilation errors and conflicts

Clean JInteropFieldValidator logic

9624344

Merge branch 'jbal-be-rewrite' of https://github.com/ballerina-platfo…

3cda3d3

…rm/ballerina-lang into jbal-be-rewrite

Use BackendDriver with test source compilation

61c3be6

Merge pull request #21124 from Kishanthan/jbal-be-rewrite

3efa011

Clean JInteropFieldValidator and Use BackendDriver for test source compilation

Cleanup jvm interop gen related classes

f4fd5e2

Merge branch 'jbal-be-rewrite' of https://github.com/ballerina-platfo…

0ef125d

…rm/ballerina-lang into jbal-be-rewrite

manuranga and others added 2 commits March 3, 2020 18:22

Fix spotbug reported issue

1e03730

Merge pull request #21447 from manuranga/jbal-be-rewrite

82df207

Change the class loader in tests to system class loader

manuranga closed this Mar 3, 2020

riyafa reviewed Mar 3, 2020

View reviewed changes

manuranga reopened this Mar 3, 2020

riyafa reviewed Mar 4, 2020

View reviewed changes

manuranga added 4 commits March 4, 2020 10:59

Fix running negative tests

0e80a5e

Disable failing negative tests

7f47c0b

Force rerun gradle tasks to override cache temporally

61278aa

Merge remote-tracking branch 'origin/jbal-be-rewrite' into jbal-be-re…

7ae8882

…write

riyafa reviewed Mar 4, 2020

View reviewed changes

Disable failing negative tests

58faa6a

Disable gradle pre-running assemble

4cbd7c6

see travis-ci/travis-ci#8667

warunalakshitha and others added 3 commits March 4, 2020 14:40

Disable balopath test temporary

88e5a15

Disable testBaloPathCase2 testcase

09fb153

Merge pull request #21473 from warunalakshitha/disable_tests

800b24e

Disable balo path test

manuranga added 2 commits March 4, 2020 18:07

Disable cache for integration test travis build

cad8d24

Disable gradle pre-running assemble for integration test

a68378a

manuranga approved these changes Mar 4, 2020

View reviewed changes

manuranga merged commit d0b0e8a into master Mar 4, 2020

riyafa mentioned this pull request Mar 5, 2020

Migrate all bValues' stringValue implementations #21205

Closed

13 tasks

rasika mentioned this pull request Mar 5, 2020

Disable force rerun gradle tasks from travis.yaml #21485

Merged

13 tasks

sameerajayasoma deleted the jbal-be-rewrite branch August 28, 2020 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jBallerina Compiler Backend Rewrite #21435

jBallerina Compiler Backend Rewrite #21435

Kishanthan commented Mar 3, 2020 •

edited

Loading

riyafa Mar 3, 2020

riyafa Mar 4, 2020

riyafa Mar 4, 2020

riyafa Mar 4, 2020

riyafa Mar 4, 2020 •

edited

Loading

riyafa Mar 4, 2020

manuranga Mar 4, 2020 •

edited

Loading

riyafa Mar 4, 2020 •

edited

Loading

manuranga Mar 4, 2020 •

edited

Loading

riyafa Mar 4, 2020

riyafa Mar 4, 2020

riyafa Mar 4, 2020

riyafa Mar 4, 2020

anupama-pathirage commented Mar 4, 2020

riyafa commented Mar 4, 2020

manuranga commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading

riyafa commented Mar 5, 2020 •

edited

Loading

	package org.wso2.ballerinalang.compiler.bir;

	package org.wso2.ballerinalang.compiler.bir;


		public static void generateBToJCheckCast(MethodVisitor mv, BType sourceType, JType targetType) {

		if (targetType.jTag == JTypeTags.JBYTE) {


		private static void generateCheckCastBToJChar(MethodVisitor mv, BType sourceType) {

		if (sourceType.tag == TypeTags.BYTE) {

jBallerina Compiler Backend Rewrite #21435

jBallerina Compiler Backend Rewrite #21435

Conversation

Kishanthan commented Mar 3, 2020 • edited Loading

Purpose

Approach

Samples

Remarks

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyafa Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuranga Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

riyafa Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

manuranga Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anupama-pathirage commented Mar 4, 2020

riyafa commented Mar 4, 2020

manuranga commented Mar 4, 2020 • edited Loading

codecov-io commented Mar 4, 2020 • edited Loading

Codecov Report

riyafa commented Mar 5, 2020 • edited Loading

Kishanthan commented Mar 3, 2020 •

edited

Loading

riyafa Mar 4, 2020 •

edited

Loading

manuranga Mar 4, 2020 •

edited

Loading

riyafa Mar 4, 2020 •

edited

Loading

manuranga Mar 4, 2020 •

edited

Loading

manuranga commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading

riyafa commented Mar 5, 2020 •

edited

Loading