[stdlib] mark dict entry as destroyed in `Dict.pop()` #2796

helehex · 2024-05-22T23:13:43Z

see #2756
this is an attempt to fix errors that sometimes arise when using the pop method. I'm not sure this is the best way, so feedback is much appreciated

helehex · 2024-05-22T23:17:32Z

ok, tests passed locally, but test_reversed is still failing, so this fix may not work

JoeLoser · 2024-05-22T23:31:21Z

stdlib/src/collections/dict.mojo

-            return entry_value.value^
+            _ = entry_value.key^
+            var value = entry_value.value^
+            __mlir_op.`lit.ownership.mark_destroyed`(


@Mogball @lattner is this the "blessed" way to do this right now?

i imagine it's not since the test_reversed still failed... maybe it's not related, but i was really hoping it was

it did fix a crash locally for me though. (the exmaple given in #2756)

gabrieldemarmiesse · 2024-06-10T11:40:58Z

ok, tests passed locally, but test_reversed is still failing, so this fix may not work

@helehex I made a PR recently fixing the flakyness in test_reverse.mojo. Can you rebase this PR on nightly and see if that works now?

rd4com · 2024-06-16T01:19:49Z

@helehex, based on #2822 last comment,
and that @gabrieldemarmiesse fixed the nasty bug,
PR should now work !

helehex · 2024-07-21T13:57:31Z

Anyone know why this cant go through? last time I checked, pop() still returns garbage data.
@JoeLoser

JoeLoser · 2024-07-21T14:59:44Z

Anyone know why this cant go through? last time I checked, pop() still returns garbage data. @JoeLoser

Is there still an issue with pop on latest nightly? Originally, we were seeing failures around the time you opened this PR, but I thought I remember @gabrieldemarmiesse chasing down the root cause and fixing it.

helehex · 2024-07-21T15:51:37Z

I just checked, and the test provided here still fails in the latest nightly.
They were unrelated. If I remember correctly, the issue here is that the entry.value gets destroyed twice.

helehex · 2024-07-21T16:28:52Z

Sorry, after reading through this thread again, I realize I should've communicated better what was going on.

helehex · 2024-07-21T17:10:13Z

Here's a few notes:

I moved the functionality into a helper method on DictEntry, after seeing Chris's response in #2822 I called it reap_value(), It makes sense conceptually to me with how it passes itself as owned.
The fix Gabriel provided allowed the tests to pass in this branch, but was unrelated to the particular issue.
What the issues basically comes down to, as far as I can tell right now, is a segfault when doing something like this:
```
var entry = DictEntry[String, String]("a", "b")
print(entry.value^)
```
which also crashes the lsp currently, maybe it's trying to give me an error?

rd4com · 2024-07-21T18:07:44Z

Yes, this PR would make it possible the move a value out of the dictionary.
(V of DictEntry[K,V] is not an Optional)

I think that what we return now is a weird copy that gets double freed.
(No problem when V is a trivial type like Int)

If we turn V into an Optional,

we'd have an overhead of the_dict._reserved() * 8 bytes.
(If V is Int, this is huge, even None entries would have the overhead)

So this PR move the value out by scavaging an DictEntry[K,V] with the blessed thing.

The problem that could be a bug (without PR)

only fully initialized instances can have their destructor used

Currently, we move the value out, and DictEntry is destroyed anyway:

var entry_value = entry[].unsafe_take() #DictEntry
...
return entry_value.value^

So if the user received an instance of a type that has UnsafePointer as fields,
The pointers are probably freed.

helehex · 2024-07-26T14:08:00Z

!sync

JoeLoser · 2024-07-26T14:10:44Z

stdlib/src/collections/dict.mojo

@@ -216,6 +216,15 @@ struct DictEntry[K: KeyElement, V: CollectionElement](
        self.key = other.key
        self.value = other.value

+    fn reap_value(owned self) -> V:


Should we make this private? I don't expect users to need to interact with this API directly, right?

hmm I'm not sure actually.

def main(): print(str(take())) def take() -> String: var entry = DictEntry[String, String]("a", "b") return entry.value^

Doing this still causes a segfault and the language server to crash.. do you know if this is a bug, or maybe it should give me an error in the editor?

As things are right now, it seems useful to have the function available and documented.

Adding __mlir_op.`lit.ownership.mark_destroyed`(__get_mvalue_as_litref(entry)) after entry is the solution to it, but that's definitely a private function.

No, I don't think this should be private right now.

…ruction of entry_value in `Dict.pop()` Signed-off-by: Max Brylski <[email protected]>

JoeLoser · 2024-08-09T22:39:23Z

!sync

modularbot · 2024-08-09T23:14:35Z

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

…897) [External] [stdlib] mark dict entry as destroyed in `Dict.pop()` Fixes #2756 Co-authored-by: Helehex <[email protected]> Closes #2796 MODULAR_ORIG_COMMIT_REV_ID: d9ee05077d11b2b4b54bdd48d1a67100ab624eee

modularbot · 2024-08-10T05:52:29Z

Landed in 486ff59! Thank you for your contribution 🎉

…897) [External] [stdlib] mark dict entry as destroyed in `Dict.pop()` Fixes modularml#2756 Co-authored-by: Helehex <[email protected]> Closes modularml#2796 MODULAR_ORIG_COMMIT_REV_ID: d9ee05077d11b2b4b54bdd48d1a67100ab624eee Signed-off-by: Manuel Saelices <[email protected]>

…897) [External] [stdlib] mark dict entry as destroyed in `Dict.pop()` Fixes #2756 Co-authored-by: Helehex <[email protected]> Closes #2796 MODULAR_ORIG_COMMIT_REV_ID: d9ee05077d11b2b4b54bdd48d1a67100ab624eee

helehex requested a review from a team as a code owner May 22, 2024 23:13

JoeLoser reviewed May 22, 2024

View reviewed changes

rd4com mentioned this pull request Jun 16, 2024

[stdlib] Fix draft for 2756 (not to merge), changes DictEntry.value:V to DictEntry.value: Optional[V] #2851

Closed

helehex force-pushed the fix/dict-pop branch 3 times, most recently from 5f7e64b to e586986 Compare June 18, 2024 02:57

rd4com mentioned this pull request Jul 1, 2024

[stdlib] Add Dict._resize_down and _under_load_factor() #3133

Open

helehex force-pushed the fix/dict-pop branch from e586986 to b6d81dd Compare July 7, 2024 21:46

helehex force-pushed the fix/dict-pop branch from b6d81dd to d9a5c8b Compare July 21, 2024 13:55

helehex force-pushed the fix/dict-pop branch 3 times, most recently from 666e623 to f25d09c Compare July 26, 2024 13:58

JoeLoser reviewed Jul 26, 2024

View reviewed changes

add reap_value() method to DictEntry, and use it to ensure the dest…

841053d

…ruction of entry_value in `Dict.pop()` Signed-off-by: Max Brylski <[email protected]>

helehex force-pushed the fix/dict-pop branch from f25d09c to 841053d Compare July 30, 2024 22:17

modular-automation bot assigned JoeLoser Aug 9, 2024

modularbot added the imported-internally Signals that a given pull request has been imported internally. label Aug 9, 2024

modularbot added the merged-internally Indicates that this pull request has been merged internally label Aug 9, 2024

modularbot added the merged-externally Merged externally in public mojo repo label Aug 10, 2024

modularbot closed this Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] mark dict entry as destroyed in `Dict.pop()` #2796

[stdlib] mark dict entry as destroyed in `Dict.pop()` #2796

helehex commented May 22, 2024

helehex commented May 22, 2024

JoeLoser May 22, 2024

helehex May 22, 2024

helehex May 22, 2024

gabrieldemarmiesse commented Jun 10, 2024

rd4com commented Jun 16, 2024 •

edited

Loading

helehex commented Jul 21, 2024 •

edited

Loading

JoeLoser commented Jul 21, 2024

helehex commented Jul 21, 2024

helehex commented Jul 21, 2024

helehex commented Jul 21, 2024 •

edited

Loading

rd4com commented Jul 21, 2024 •

edited

Loading

helehex commented Jul 26, 2024

JoeLoser Jul 26, 2024

helehex Jul 26, 2024

helehex Jul 26, 2024

helehex Jul 26, 2024

helehex Jul 30, 2024

JoeLoser commented Aug 9, 2024

modularbot commented Aug 9, 2024

modularbot commented Aug 10, 2024

[stdlib] mark dict entry as destroyed in Dict.pop() #2796

[stdlib] mark dict entry as destroyed in Dict.pop() #2796

Conversation

helehex commented May 22, 2024

helehex commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldemarmiesse commented Jun 10, 2024

rd4com commented Jun 16, 2024 • edited Loading

helehex commented Jul 21, 2024 • edited Loading

JoeLoser commented Jul 21, 2024

helehex commented Jul 21, 2024

helehex commented Jul 21, 2024

helehex commented Jul 21, 2024 • edited Loading

rd4com commented Jul 21, 2024 • edited Loading

The problem that could be a bug (without PR)

helehex commented Jul 26, 2024

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

JoeLoser commented Aug 9, 2024

modularbot commented Aug 9, 2024

modularbot commented Aug 10, 2024

[stdlib] mark dict entry as destroyed in `Dict.pop()` #2796

[stdlib] mark dict entry as destroyed in `Dict.pop()` #2796

rd4com commented Jun 16, 2024 •

edited

Loading

helehex commented Jul 21, 2024 •

edited

Loading

helehex commented Jul 21, 2024 •

edited

Loading

rd4com commented Jul 21, 2024 •

edited

Loading