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

[Bug] RingsDetection test did not meet expectations #275

Closed
1 task done
diaohancai opened this issue Oct 25, 2023 · 5 comments · Fixed by #276
Closed
1 task done

[Bug] RingsDetection test did not meet expectations #275

diaohancai opened this issue Oct 25, 2023 · 5 comments · Fixed by #276
Assignees
Labels
bug Something isn't working

Comments

@diaohancai
Copy link
Contributor

diaohancai commented Oct 25, 2023

Bug Type (问题类型)

data inconsistency (计算结果不合预期)

Before submit

Environment (环境信息)

  • Server Version: v1.0.x
  • Computer Version: v1.0.x

Expected & Actual behavior (期望与实际表现)

Run org.apache.hugegraph.computer.algorithm.path.rings.RingsDetectionTest#testRunAlgorithm
Vertex A expected result is:

[[A, C, A], [A, D, A], [A, B, C, A], [A, D, C, A], [A, C, E, D, A], [A, B, C, E, D, A]]

But got:

[[A, D, A], [A, D, A], [A, D, C, A], [A, D, C, A], [A, C, E, D, A], [A, B, C, E, D, A]]

I modified some code and it works.

    @Override
    public void compute(ComputationContext context, Vertex vertex,
                        Iterator<IdList> messages) {
        Id id = vertex.id();
        boolean halt = true;
        while (messages.hasNext()) {
            halt = false;
            // IdList sequence = messages.next();  // Something is wrong here
            IdList sequence = messages.next().copy(); // copy() is good
            ...

Further debug, I found messages.next() always return the same address Object(Field value may be different)!!!, it's a reference Object.

Semantically, I think messages.next() should return a different address Object.

@diaohancai diaohancai added the bug Something isn't working label Oct 25, 2023
@diaohancai
Copy link
Contributor Author

Possible solutions:
org.apache.hugegraph.computer.core.compute.input.MessageInput

        @Override
        public boolean hasNext() {
            if (this.valueValid) {
                return true;
            }
            if (MessageInput.this.messages.hasNext()) {
                KvEntry entry = MessageInput.this.messages.peek();
                Pointer key = entry.key();
                int status = this.vidPointer.compareTo(key);
                if (status == 0) {
                    MessageInput.this.messages.next();
                    this.valueValid = true;
                    try {
                        BytesInput in = IOFactory.createBytesInput(
                                        entry.value().bytes());
                        // create a new value
                        MessageInput.this.value = MessageInput.this.config
                                .createObject(ComputerOptions.ALGORITHM_MESSAGE_CLASS);
                        MessageInput.this.value.read(in);

messages.next() return a different address Object.

@corgiboygsj
Copy link
Member

Possible solutions: org.apache.hugegraph.computer.core.compute.input.MessageInput

        @Override
        public boolean hasNext() {
            if (this.valueValid) {
                return true;
            }
            if (MessageInput.this.messages.hasNext()) {
                KvEntry entry = MessageInput.this.messages.peek();
                Pointer key = entry.key();
                int status = this.vidPointer.compareTo(key);
                if (status == 0) {
                    MessageInput.this.messages.next();
                    this.valueValid = true;
                    try {
                        BytesInput in = IOFactory.createBytesInput(
                                        entry.value().bytes());
                        // create a new value
                        MessageInput.this.value = MessageInput.this.config
                                .createObject(ComputerOptions.ALGORITHM_MESSAGE_CLASS);
                        MessageInput.this.value.read(in);

messages.next() return a different address Object.

always return the same Object is reduce memory pressure and avoid GC

@diaohancai
Copy link
Contributor Author

always return the same Object is reduce memory pressure and avoid GC

Yep~ But semantically, messages.next() returns a different address object more intuitive.
Otherwise, someone who call this function downstream need to clearly know that the same object is returned here, which is not logically intuitive and easy to make mistakes.

Do we have some more elegant solutions?

@corgiboygsj
Copy link
Member

@javeme please take a look for this question. This does make it confusing for users.

@javeme
Copy link
Contributor

javeme commented Oct 29, 2023

I agree this is a problem, it sacrifices user-friendliness for performance.
We hope that after the memory management module is added, this confusion can be solved.
it's tracked by #277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants