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

[Task]: Revamp cloneWithType and fromJsonWithType #39217

Closed
10 tasks done
Nadeeshan96 opened this issue Jan 10, 2023 · 5 comments
Closed
10 tasks done

[Task]: Revamp cloneWithType and fromJsonWithType #39217

Nadeeshan96 opened this issue Jan 10, 2023 · 5 comments
Assignees
Labels
Team/jBallerina All the issues related to BIR, JVM backend code generation and runtime Type/Task

Comments

@Nadeeshan96 Nadeeshan96 added Type/Task Team/jBallerina All the issues related to BIR, JVM backend code generation and runtime labels Jan 10, 2023
@Nadeeshan96 Nadeeshan96 self-assigned this Jan 10, 2023
@Nadeeshan96
Copy link
Contributor Author

Tried to do a performance improvement in the TypeConverter.getConvertibleType type method by saving the resolved convertible types in a hash map. Then compared the performance before and after the change.

First ran the langlib value tests in a large loop and measured the time and memory using Jprofiler.

Before After
Peak memory usage 236.7 MB 247.6 MB (+4.6%)
Total all objects live memory 250 MB 185 MB (-26%)
Total time 32 s 34 s (+6.25%)
convert method total time 14.4 s 16.57 s (+15.07%)
Peak process CPU load 37.56% 33.27%

Then converted a large json to a record in a large loop and measured the time and memory using Jprofiler.

Before After
Peak memory usage 126.4 MB 212.5 MB (+68.12%)
Total all objects live memory 195 MB 75.7 MB (-61.18%)
Total time 11 s 8 s (-27.27%)
convert method total time 9.303 s 7.304 s (-21.49%)
Peak process CPU load 26.7% 27%

Due to the above results, decided not to do the above change.

@Nadeeshan96
Copy link
Contributor Author

Nadeeshan96 commented Jan 31, 2023

Improve error message given in the failure of type conversion when the target type is a union

public function main() {
    foo();
}

type NumberArray int[]|float[];
type StringArray string[]|string:Char[];
type NumberOrStringArray NumberArray|StringArray;

type RecordWithArrayAsAField record {|
    NumberOrStringArray arr;
|};

type RecordWithNumberArrayAsAField record {|
    NumberArray arr;
|};

function foo() {
    json j = {"arr": [1,1.2,"a","aa"]};
    RecordWithArrayAsAField|RecordWithNumberArrayAsAField jCloned = checkpanic j.cloneWithType();
}

For example for the above code the error message is given as follows with all the errors together, which seems infeasible.

Running executable

error: {ballerina/lang.value}ConversionError {"message":"'map<json>' value cannot be converted to '(RecordWithArrayAsAField|RecordWithNumberArrayAsAField)': 
                array element 'arr[2]' should be of type 'int', found '"a"'
                array element 'arr[3]' should be of type 'int', found '"aa"'
                array element 'arr[2]' should be of type 'float', found '"a"'
                array element 'arr[3]' should be of type 'float', found '"aa"'
                array element 'arr[0]' should be of type 'string', found '1'
                array element 'arr[1]' should be of type 'string', found '1.2'
                array element 'arr[0]' should be of type 'lang.string:Char', found '1'
                array element 'arr[1]' should be of type 'lang.string:Char', found '1.2'
                array element 'arr[3]' should be of type 'lang.string:Char', found '"aa"'
                array element 'arr[2]' should be of type 'int', found '"a"'
                array element 'arr[3]' should be of type 'int', found '"aa"'
                array element 'arr[2]' should be of type 'float', found '"a"'
                array element 'arr[3]' should be of type 'float', found '"aa"'"}
        at ballerina.lang.value.0:cloneWithType(value.bal:114)
           temp:foo(temp.bal:23)
           temp:main(temp.bal:6)

This can be improved for something like below.

Running executable

error: {ballerina/lang.value}ConversionError {"message":"'map<json>' value cannot be converted to '(RecordWithArrayAsAField|RecordWithNumberArrayAsAField)': 
                    array element 'arr[2]' should be of type 'int', found '"a"'
                    array element 'arr[3]' should be of type 'int', found '"aa"'
                  or
                    array element 'arr[2]' should be of type 'float', found '"a"'
                    array element 'arr[3]' should be of type 'float', found '"aa"'
                  or
                    array element 'arr[0]' should be of type 'string', found '1'
                    array element 'arr[1]' should be of type 'string', found '1.2'
                  or
                    array element 'arr[0]' should be of type 'lang.string:Char', found '1'
                    array element 'arr[1]' should be of type 'lang.string:Char', found '1.2'
                    array element 'arr[3]' should be of type 'lang.string:Char', found '"aa"'
                or
                    array element 'arr[2]' should be of type 'int', found '"a"'
                    array element 'arr[3]' should be of type 'int', found '"aa"'
                  or
                    array element 'arr[2]' should be of type 'float', found '"a"'
                    array element 'arr[3]' should be of type 'float', found '"aa"'"}
        at ballerina.lang.value.0:cloneWithType(value.bal:114)
           temp:foo(temp.bal:23)
           temp:main(temp.bal:6)

@Nadeeshan96
Copy link
Contributor Author

Another bug:

type JsonUnion xml|json;
type AnyDataUnion int|anydata;

public function main() {
    xml a  = xml `<a>1</a>`;
    // JsonUnion & readonly j = checkpanic a.cloneWithType();

    AnyDataUnion & readonly jj = checkpanic a.cloneWithType();
}

gives

error: {ballerina}TypeCastError {"message":"incompatible types: 'lang.xml:Element' cannot be cast to 'error'"}
        at jsonUnuion:main(jsonUnuion.bal:8)

@Nadeeshan96
Copy link
Contributor Author

Nadeeshan96 commented Mar 20, 2023

#39217 (comment) and #39217 (comment) is fixed with #39212.

@Nadeeshan96
Copy link
Contributor Author

Closing the issue because the task is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/jBallerina All the issues related to BIR, JVM backend code generation and runtime Type/Task
Projects
None yet
Development

No branches or pull requests

1 participant