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

Fix Fetch and OpDeref #467

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Fix Fetch and OpDeref #467

merged 2 commits into from
Nov 16, 2023

Conversation

a-peyrard
Copy link
Contributor

First commit is fixing some issue with runtime.Deref which is called by OpDeref, pointer of interface were not de-referenced.
Second commit is creating a fix for #466. This is de-referencing everything in Fetch before fetching the attribute.
As discussed in the issue, one other approach could be to ensure we have a OpDeref operation added during the compilation phase.

Loop over `Ptr` and `Interface` kind and get `Elem`. Also return
directly the `nil` otherwise we might are returning the pointer nil
value and not the base nil.
We were only de-referencing pointers, but we can have pointer of
interface or interface obfuscating pointers...
So before trying to fetch anything, just deference fully the
variable.
@a-peyrard
Copy link
Contributor Author

I will have a look tomorrow to see if I can understand where I would need to modify the code to add the OpDeref operation.
I would also like to check if the modifications are not impacting the overall performance as the two modifications are for functions used during runtime.

@antonmedv
Copy link
Member

antonmedv commented Nov 14, 2023

Results looks promising.

@antonmedv antonmedv self-requested a review November 14, 2023 17:48
@@ -28,10 +36,8 @@ func Fetch(from, i any) any {
// Structs, maps, and slices can be access through a pointer or through
// a value, when they are accessed through a pointer we don't want to
// copy them to a value.
if kind == reflect.Ptr {
v = reflect.Indirect(v)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we going to loose reflect.Indirect here.

Copy link
Member

Choose a reason for hiding this comment

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

Here is ooriginal change: 6f0c855

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still doing the "indirect" in the deref function as we are calling Elem() of the value is a pointer.
Which should be doing the same as the Indirect function, which is also doing Elem() behind the scene:

func Indirect(v Value) Value {
	if v.Kind() != Pointer {
		return v
	}
	return v.Elem()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran the benchrmak, it seems we are in the same order of magnitude:
before:

Benchmark_largeStructAccess-12           1000000               134.4 ns/op

after:

Benchmark_largeStructAccess-12           1000000               132.2 ns/op

Copy link
Member

Choose a reason for hiding this comment

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

Cool! let me check it a bit more as runtime changes should be done very carefully.

@a-peyrard
Copy link
Contributor Author

a-peyrard commented Nov 14, 2023

I was trying to figure out where we would need to add an OpDeref operation. I was thinking to add one there:
https://github.com/antonmedv/expr/blob/709c5dd55aa7cfc1b47fc67ee32ac15af6db967a/compiler/compiler.go#L590
like that:

if op == OpFetch {
		c.derefInNeeded(base)
		c.compile(node.Property)
		c.emit(OpFetch)
	}

This is fixing the problem when we try to access a field, but introducing a bug when fetch is used to get a method. This test for example is failing: TestDeref_method_on_int_pointer as the element has been dereferenced and then doesn't contain any method named Bar.

Any idea where I should add a OpDeref, as Fetch is used to fetch fields and methods, it might be a bit tricky w/o splitting the memberNode into two separate structures, one to retrieve methods, and one to retrieve fields.

The other solution would be to not add any OpDeref and just do the deref if needed during the Fetch (as implemented in this PR)

@antonmedv
Copy link
Member

The other solution would be to not add any OpDeref and just do the deref if needed during the Fetch (as implemented in this PR)

Let's use this for now.

@antonmedv
Copy link
Member

The other solution would be to not add any OpDeref and just do the deref if needed during the Fetch (as implemented in this PR)

Lets use this for now.

@antonmedv antonmedv merged commit 7f07463 into expr-lang:master Nov 16, 2023
8 checks passed
@a-peyrard a-peyrard deleted the fix-deref branch November 16, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants