Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Add a lot more documentation to STM and structure it better #302

Merged
merged 14 commits into from Nov 29, 2020

Conversation

1Jajen1
Copy link
Member

@1Jajen1 1Jajen1 commented Nov 13, 2020

So after seeing the rendered api docs for STM I wasn't quite satisfied. It was really hard to see which methods worked on which datatype and what the datatypes actually did.

This hopefully addresses that issue by documenting the datatypes with the methods used to operate on them. It also adds a ton of simple examples and changes/adds a few operator overload methods to be more like the stdlib collections.

Edit: This is 95-99% documentation btw^^ So don't be scared by +2000 here 馃檲

@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 13, 2020

Oh and also: Is there a place in the arrow-fx docs sidebar that we can feature STM in? Apidocs is ok-ish but adding it to like datatypes as well would be a bit better imo. Just not sure it fits there.

@nomisRev
Copy link
Member

@1Jajen1 need to make some time to go through this, but feel free to restructure the Arrow Fx website a bit to make room for STM in the sidebar. Perhaps we need to relocate data types under an Arrow Fx Coroutines specific section?
We might also want to make room for more modules later on in a similar style.

https://github.com/arrow-kt/arrow-site/blob/master/docs/_data/sidebar-fx.yml

@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 13, 2020

We might also want to make room for more modules later on in a similar style.

Likely as this will apply to Stream as well.

I'll try to think of something, but as of now I don't have a good idea what the category for STM (or Stream) should be...

@nomisRev
Copy link
Member

We could change the Data types section to Arrow Fx Coroutines, and we can add a section for STM and Stream?
I am working on the split of Streams atm ;)

No clue how this got in there
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Great addition to the docs. Some suggestions, which leads to a bit to duplication but I think that's okay!

arrow-fx-stm/src/main/kotlin/arrow/fx/stm/STM.kt Outdated Show resolved Hide resolved
arrow-fx-stm/src/main/kotlin/arrow/fx/stm/STM.kt Outdated Show resolved Hide resolved
arrow-fx-stm/src/main/kotlin/arrow/fx/stm/TMVar.kt Outdated Show resolved Hide resolved
arrow-fx-stm/src/main/kotlin/arrow/fx/stm/TQueue.kt Outdated Show resolved Hide resolved
@nomisRev nomisRev requested a review from a team November 21, 2020 10:04
@nomisRev
Copy link
Member

A second review is needed @arrow-kt/maintainers

Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

This is great, thanks so much @1Jajen1 and @nomisRev for taking the time and reviewing. I requested some changes regarding type names and operator names to make it closer to our efforts to be inline with the std lib semantics and make some types like TSem more exploit if possible.

* well distributed the hash function is), it also means that structural changes can be isolated and thus do not increase contention with
* other transactions. This effectively means concurrent access to different values is unlikely to interfere with each other.
*
* > Hash conflicts are resolved by chaining.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean by chaining?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two ways to to resolve hash conflicts:

  • Either replace the value and assume hashes are unique
  • Or put them in a collection and iterate through them on lookups

Chaining here refers to chaining it to the end of the leaf array, I've heard and seen it used in this context while I did a bit of research on how Hamt structures work so I just used it here.

arrow-fx-stm/src/main/kotlin/arrow/fx/stm/TMap.kt Outdated Show resolved Hide resolved
* val tq = TQueue.new<Int>()
* atomically {
* tq.write(0)
* tq.filter { it != 0 }
Copy link
Member

Choose a reason for hiding this comment

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

filter as a name is confusing if it does not return a new queue as users may be used ro. Is there a better name for this op?

Copy link
Member Author

@1Jajen1 1Jajen1 Nov 26, 2020

Choose a reason for hiding this comment

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

I honestly have no idea, it really does filter, so dunno. Actually I really dislike the filter as an api because it a) increases contention (accesses both TVars) and b) it has a weird/different api as you mentioned.

Maybe it makes sense to just 馃敟 it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe applyFilter to highlight that it directly applies to the TQueue?

Copy link
Member

Choose a reason for hiding this comment

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

If you dislike the function, I am fine with removing it. It can always be re-added later, removing is harder 馃榿
I agree the name is a bit confusing.
Perhaps something along the line of remove/removeAll? That's a function found in the Kotlin Standard library but only available for mutable collections.

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/remove.html
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/remove-all.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I think keeping it is fine, removeAll with a predicate is a really good name. It still has the problem of contention but it may have some actual use, so long as its used infrequently its perfectly fine.

arrow-fx-stm/src/main/kotlin/arrow/fx/stm/TSem.kt Outdated Show resolved Hide resolved
@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 26, 2020

I requested some changes regarding type names and operator names to make it closer to our efforts to be inline with the std lib semantics and make some types like TSem more exploit if possible.

Yes please, now is the perfect time to align the api, I took the names from the haskell libs and related so any change is fine by me. I'll do it in this pr if that is fine with you.

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

馃憣

@PhBastiani
Copy link
Contributor

I'm starting to read the doc ... I'm impressed with the work you have done for Continuations & STM 馃憦

A README explaining the main functionalities would be useful to introduce the purpose of the module.

@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 28, 2020

I'm starting to read the doc ... I'm impressed with the work you have done for Continuations & STM

A README explaining the main functionalities would be useful to introduce the purpose of the module.

Thanks for reading it 馃檹

The README: Regarding the implementation or the api function? I suppose a readme describing how the implementation works ala fx-coroutines makes most sense as the rest should be in the docs^^

@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 28, 2020

I'm starting to read the doc ... I'm impressed with the work you have done for Continuations & STM
A README explaining the main functionalities would be useful to introduce the purpose of the module.

Thanks for reading it pray

The README: Regarding the implementation or the api function? I suppose a readme describing how the implementation works ala fx-coroutines makes most sense as the rest should be in the docs^^

Either way I think its best left for another pr as this one is quite close to completion. For a general introduction to what STM can do the api docs for the STM interface and the documentation for atomically are the best start.

@PhBastiani
Copy link
Contributor

PhBastiani commented Nov 28, 2020

Either way I think its best left for another pr as this one is quite close to completion. For a general introduction to what STM can do the api docs for the STM interface and the documentation for atomically are the best start.

We don't need a long README: a short introduction would be sufficient, as long as the reader is encouraged to read the STM class doc (I mean it is important to mark the main entry point of the API).

@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 28, 2020

We don't need a long README: a short introduction would be sufficient, as long as the reader is encouraged to read the STM class doc (I mean it is important to mark the main entry point of the API).

Ah ok, I was confused by what exactly you were referring to. I'll add that in this pr, thanks for the suggestion! At some point I'll add a section to it which explains the internals as a sort of contribution guide like arrow-fx-coroutines does atm, but thats a later pr.

@nomisRev
Copy link
Member

At some point I'll add a section to it which explains the internals as a sort of contribution guide like arrow-fx-coroutines does atm, but thats a later pr.
鉂わ笍

Very very simple way of pointing ppl towards to docs
@1Jajen1
Copy link
Member Author

1Jajen1 commented Nov 29, 2020

Added a readme which contains a link to the STM interface api docs. We still need a section in the sidebar, but until I have a decent idea where, that can wait.

If there is nothing else I'll merge this pr later today 馃帀

@1Jajen1 1Jajen1 merged commit 642b442 into master Nov 29, 2020
@1Jajen1 1Jajen1 deleted the jo-stm-docs branch November 29, 2020 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants