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

gc_alloc() - should collected=1 after gc_collect() is executed? #3786

Closed
adritium opened this issue May 16, 2018 · 2 comments
Closed

gc_alloc() - should collected=1 after gc_collect() is executed? #3786

adritium opened this issue May 16, 2018 · 2 comments

Comments

@adritium
Copy link

adritium commented May 16, 2018

micropython/py/gc.c

Lines 452 to 458 in 1b7487e

#if MICROPY_GC_ALLOC_THRESHOLD
if (!collected && MP_STATE_MEM(gc_alloc_amount) >= MP_STATE_MEM(gc_alloc_threshold)) {
GC_EXIT();
gc_collect();
GC_ENTER();
}
#endif

If a gc_collect() is performed here then why isn't collected set to 1 just like it is in the next code block?

micropython/py/gc.c

Lines 471 to 479 in 1b7487e

GC_EXIT();
// nothing found!
if (collected) {
return NULL;
}
DEBUG_printf("gc_alloc(" UINT_FMT "): no free mem, triggering GC\n", n_bytes);
gc_collect();
collected = 1;
GC_ENTER();

Under existing code, it seems you would

  1. Perform a gc_collect() (from L455)
  2. Attempt to locate a large enough block
  3. If large enough block not found in step 2, perform another gc_collect() which would be 100% pointless because no allocation operation was performed between step 2 and step 3 so it's guaranteed the memory layout hasn't changed.
  4. Set collected = 1
  5. Look again for a large enough block as in step 2 - since it failed the first time, it will fail again because memory layout has not change since the first time gc_collect() was called on L455
  6. If large enough block not found, return NULL

By adding collected = 1 at L455, we'd be avoiding an extra gc_collect() + search.

@dpgeorge
Copy link
Member

By adding collected = 1 at L455, we'd be avoiding an extra gc_collect() + search.

Yes, that looks to be a correct conclusion.

@dpgeorge
Copy link
Member

Fixed by 6bd7874

dpgeorge added a commit that referenced this issue May 21, 2018
Without this, if GC threshold is hit and there is not enough memory left to
satisfy the request, gc_collect() will run a second time and the search for
memory will happen again and will fail again.

Thanks to @adritium for pointing out this issue, see #3786.
tannewt added a commit to tannewt/circuitpython that referenced this issue Dec 8, 2020
This changes lots of files to unify `board.h` across ports. It adds
`board_deinit` when CIRCUITPY_ALARM is set. `main.c` uses it to
deinit the board before deep sleeping (even when pretending.)

Deep sleep is now a two step process for the port. First, the
port should prepare to deep sleep based on the given alarms. It
should set alarms for both deep and pretend sleep. In particular,
the pretend versions should be set immediately so that we don't
miss an alarm as we shutdown. These alarms should also wake from
`port_idle_until_interrupt` which is used when pretending to deep
sleep.

Second, when real deep sleeping, `alarm_enter_deep_sleep` is called.
The port should set any alarms it didn't during prepare based on
data it saved internally during prepare.

ESP32-S2 sleep is a bit reorganized to locate more logic with
TimeAlarm. This will help it scale to more alarm types.

Fixes micropython#3786
tannewt added a commit to tannewt/circuitpython that referenced this issue Dec 8, 2020
This changes lots of files to unify `board.h` across ports. It adds
`board_deinit` when CIRCUITPY_ALARM is set. `main.c` uses it to
deinit the board before deep sleeping (even when pretending.)

Deep sleep is now a two step process for the port. First, the
port should prepare to deep sleep based on the given alarms. It
should set alarms for both deep and pretend sleep. In particular,
the pretend versions should be set immediately so that we don't
miss an alarm as we shutdown. These alarms should also wake from
`port_idle_until_interrupt` which is used when pretending to deep
sleep.

Second, when real deep sleeping, `alarm_enter_deep_sleep` is called.
The port should set any alarms it didn't during prepare based on
data it saved internally during prepare.

ESP32-S2 sleep is a bit reorganized to locate more logic with
TimeAlarm. This will help it scale to more alarm types.

Fixes micropython#3786
tannewt added a commit to tannewt/circuitpython that referenced this issue Dec 8, 2020
This changes lots of files to unify `board.h` across ports. It adds
`board_deinit` when CIRCUITPY_ALARM is set. `main.c` uses it to
deinit the board before deep sleeping (even when pretending.)

Deep sleep is now a two step process for the port. First, the
port should prepare to deep sleep based on the given alarms. It
should set alarms for both deep and pretend sleep. In particular,
the pretend versions should be set immediately so that we don't
miss an alarm as we shutdown. These alarms should also wake from
`port_idle_until_interrupt` which is used when pretending to deep
sleep.

Second, when real deep sleeping, `alarm_enter_deep_sleep` is called.
The port should set any alarms it didn't during prepare based on
data it saved internally during prepare.

ESP32-S2 sleep is a bit reorganized to locate more logic with
TimeAlarm. This will help it scale to more alarm types.

Fixes micropython#3786
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

No branches or pull requests

2 participants