Skip to content

Conversation

@kfarnung
Copy link
Contributor

Some objects, like tagged integers, aren't allocated on the heap. In this case there's no strongRefHeapBlock associated with the weak reference.

@MSLaguana
Copy link
Contributor

The fix looks reasonable to me in terms of correctness.

However I wonder if there's another change to completely avoid creating a weak ref entry for tagged numbers, since they will never be collected. Perhaps we should modify the Create method to return early on tagged values? A quick look suggests that won't be trivial though since all the functions that call Create expect to get back a non-null value, and some things access it immediately.

@kfarnung
Copy link
Contributor Author

Yeah, I agree with that. I just wanted to see if this solution would work, and it appears to resolve the issue in node as well. I'll mark the node test flaky and open an issue to figure out the weak ref problem in node-chakracore. It would be interesting to run some benchmarks on this change to make sure there are no regressions, but I don't think I'll be able to get around to that myself.

@curtisman
Copy link
Contributor

I prefer not creating the weak ref of non GC pointers in the first place if possible as well.

@MSLaguana
Copy link
Contributor

A simpler fix might be to put the "is it a tagged value" logic in the JSRT layer, and assume/enforce that internally we never attempt to create weak references to tagged values. That would solve the issue for node and avoid these dead weak reference entries lingering.

@digitalinfinity
Copy link
Contributor

Agree with @curtisman and @MSLaguana here- we shouldn't be creating weak references to non-GC pointers in the first place...

@kfarnung
Copy link
Contributor Author

kfarnung commented Sep 4, 2017

Sounds good to me, I'll close this out.

@kfarnung kfarnung closed this Sep 4, 2017
@kfarnung kfarnung deleted the taggedintweakref branch April 16, 2018 21:16
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.

5 participants