4 ms·
The results can be ignored. You can't just copy-paste ChatGPT without even a sanity check e.g., Python code is wrong: await sleep(10) sleeps 10+ seconds a
by d0mine 3y ago
The results can be ignored.
You can't just copy-paste ChatGPT without even a sanity check e.g., Python code is wrong:
await sleep(10)
sleeps 10+ seconds and returns None but the intent is to pass a coroutine to create_task() (await should be removed at the very least)
- pkolaczk 3y agoOk, this one is funny. Actually ChatGPT got it right and there was a create_task before, but right before committing I "inlined it" and introduced a bug. Thanks for letting me know. Will double check if it didn't messed up the results. And BTW - I'm very far from copy pasting from chat gpt. ChatGPT helped with this code, but often required many iterations and also some manual cleanup.
- mrj 3y agoYeah Python got done dirty here. I tried a million tasks and I saw a peak of about 166MB. I'm still getting used to async in Python, but this seems to work fine though? import asyncio async def do_task(): await asyncio.sleep(10) async def main(num_tasks: int): tasks = [] for task_id in range(num_tasks): task = asyncio.create_task(do_task()) tasks.append(task) print("spawned tasks") await asyncio.gather(*tasks) if __name__ == "__main__": asyncio.run(main(1_000_000))
- pkolaczk 3y agoThank you! I fixed the code and updated the benchmarks. However, your 166 MB doesn't match my testing (I'm getting about 240M at 100k tasks) and 10x more at 1M. Are you sure you haven't misread your RSS?
- mrj 3y agoNope, that's what it is for me but you're using an older Python. 3.11 has a lot of improvements and not sure why you wouldn't use the latest?
- d0mine 3y agoIf Python 3.11 is available, then asyncio.TaskGroup could be used instead of asyncio.gather() https://docs.python.org/3/library/asyncio-task.html#task-groups https://docs.python.org/3/library/asyncio-task.html#task-gro...
- pkolaczk 3y agoRunning Ubuntu LTS and that was the version from the official repo. Will try a newer version then.
- josephg 3y agoJavascript is better, but its still sloppy: tasks.push( (async () => { await delay(10000); })() ); .. So tasks.push is called with an IIFE, which returns a promise, which resolves after its internally awaited another promise... You could just do this, which avoids about 3 unnecessary allocations: tasks.push(delay(10000)) And I'm not convinced promisify(setTimeout) is what you want here. I'd just use this: const delay = timeout => new Promise(resolve => setTimeout(resolve, timeout)) ... Which honestly should be part of the standard library in javascript. However, I'm less convinced that replacing the delay function will make a big difference to performance.
- pkolaczk 3y agoThanks for the suggestion. I fixed it and indeed it improved memory consumption of NodeJS.
- josephg 3y agoHeh confirming the old adage: If you want to know the right answer to a problem online, post the wrong answer. People will come out of the woodwork to correct you. I uh, may be in this picture.
- mhio 3y agoThe article updated to remove the cruft you noted. It dropped the 1M memory substantially Promise timers are now packaged by node at least, they allocate about 20% less memory for the 1mil test (on v18). const { setTimeout } = require('node:timers/promises') https://nodejs.org/api/timers.html#timers-promises-api https://nodejs.org/api/timers.html#timers-promises-api
- josephg 3y agoOh I didn't know that! Thanks!