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

Add an ARCHITECTURE.md document #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add an ARCHITECTURE.md document #302

wants to merge 2 commits into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 23, 2024

It would be useful to have an ARCHITECTURE document that can be used by
new contributors to get a "bird's eye view" of how things are arranged
in smol. This commit adds a basic ARCHITECTURE.md document with the aim
that it can be used by prospective contributors.

So far only the lower-level items of the ecosystem have been described.

@notgull notgull requested a review from zeenix February 23, 2024 05:09
Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Great work! I've nitpicks as usual. :) Don't forget to address the TODOs btw.

ARCHITECTURE.md Outdated Show resolved Hide resolved
ARCHITECTURE.md Outdated Show resolved Hide resolved

The architecture of [`smol-rs`].

This document describes the architecture of [`smol-rs`] and its crates on a high
Copy link
Member

Choose a reason for hiding this comment

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

we follow 100 chars limit for lines in code so I think we should follow the same in markdown files. Short lines make the document look longer and people can be put off by size of the reading. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally I follow a limit for 80 chars in Markdown files, as some terminals rely on this.

Copy link
Member

Choose a reason for hiding this comment

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

These days? Which terminals? 😯

In any case, they'll have the same issues with the code. It's best to be consistent.

ARCHITECTURE.md Outdated Show resolved Hide resolved
@zeenix
Copy link
Member

zeenix commented Feb 23, 2024

BTW, instead of putting all this in a separate doc, I think it should just be here. We'll need a new repo named smol-rs.

@notgull
Copy link
Member Author

notgull commented Feb 26, 2024

BTW, instead of putting all this in a separate doc, I think it should just be here. We'll need a new repo named smol-rs.

Do you mean the .github repository? It's probably a long document for the frontpage.

@zeenix
Copy link
Member

zeenix commented Feb 26, 2024

BTW, instead of putting all this in a separate doc, I think it should just be here. We'll need a new repo named smol-rs.

Do you mean the .github repository?

AFAIK, you need to create a repository by the same name as the space (smol-rs in this case) to show the README.md from it on the front page.

It's probably a long document for the frontpage.

People don't need to read all of it. I think visibility and discoverability is more important. Having said that, I wouldn't mind a different document on the front page but until someone write that, I think this should go there.

It would be useful to have an ARCHITECTURE document that can be used by
new contributors to get a "bird's eye view" of how things are arranged
in smol. This commit adds a basic ARCHITECTURE.md document with the aim
that it can be used by prospective contributors.

So far only the lower-level items of the ecosystem have been described.

Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
@notgull
Copy link
Member Author

notgull commented May 27, 2024

People don't need to read all of it. I think visibility and discoverability is more important. Having said that, I wouldn't mind a different document on the front page but until someone write that, I think this should go there.

I've put an interim document on the frontpage for now.

@notgull notgull requested a review from fogti June 7, 2024 02:31
### [`parking`]

[`parking`] is used to block threads until an arbitrary signal is received, or
"parks" them. The [`std::thread::park`] API suffers from involving global state;
Copy link
Member

Choose a reason for hiding this comment

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

", or parks them" is worded a bit confusingly (as "parks" is/should be associated to "block threads"), but this isn't obvious here.

In order to construct runtimes, you need to be able to store [`Waker`]s in a
concurrent way. That way, different parties can simultaneously store [`Waker`]s
to show interest in an event, while activators of the event can take that
[`Waker`] can wake it. [`atomic-waker`] provides [`AtomicWaker`], which is a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[`Waker`] can wake it. [`atomic-waker`] provides [`AtomicWaker`], which is a
[`Waker`] to wake it. [`atomic-waker`] provides [`AtomicWaker`], which is a


### [`concurrent-queue`]

[`concurrent-queue`] is a fork of the [`crossbeam-queue`] crate. It provides a
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to explain why the fork happened.

Comment on lines +136 to +137
optimized single-item queues, queues with a bounded capacity, and infinite
queues with unbounded capacity.
Copy link
Member

Choose a reason for hiding this comment

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

This feels redundant.

At a low level, [`async-task`] can be seen as putting a future on the heap and
providing a handle that can be used by executors. The [`Runnable`] is the part
of the task that represents the future to be run. When the [`Runnable`] is
spawned to the existence by the coroutine indicating that it is ready, it can
Copy link
Member

Choose a reason for hiding this comment

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

This is difficult to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants