www + d: Move comments likes tally from d to www #610

fernandoabolafio · 2018-11-19T19:08:10Z

This PR removes the comment likes tally from d and add into www. It also standardizes the terminology as comment likes instead of mixing comment votes and comment likes across structs and functions.

The comment likes are all fetched from d when the www inventory is initialized and it is updated as new "comment likes" come in.
closes #603

politeiawww/comments.go

politeiawww/backend.go

politeiawww/comments.go

politeiawww/backend.go

politeiawww/inventory.go

fernandoabolafio · 2018-11-21T17:07:01Z

@lukebp I addressed all of your comments. I've also changed every reference from "comment vote" to "comment like" or the equivalent (e.g functions names, variables, etc) which required some changes on the GUI: decred/politeiagui#911.

lukebp · 2018-11-21T18:37:59Z

Code review looks good to me. I tested this PR and it is working as expected.

marcopeereboom · 2018-11-30T14:28:41Z

politeiad/backend/gitbe/decred.go

-	if decredPluginCommentsUserCache != nil {
-		seen = decredPluginCommentsUserCache
-	}
+	commentsLikes := []decredplugin.LikeComment{}


Please use make here with the somewhat correct length.

I can't make use of make here because is not possible to know the correct length because the journals haven't been replayed at this point.

Then use a PNOOMA.

From slack resolution:

fernandoabolafio [3:05 PM] ok, so 1st one: I wrote: `commentsLikes := []decredplugin.LikeComment{}` and you asked me to use `make` and estimate a length for this array. So, the total number of like comments for a proposal is really hard to estimate. So why should we allocate the memory for a random number instead of letting it allocate as needed by using what I wrote or `var commentLikes []decredplugin.LikeComment`? moo31337 [3:12 PM] because that requires a bunch of mallocs under the hood if you have no idea about size pick something large like 1024 make(x,0,1024) if you do know the size do: make(x,0,len(xxx))

marcopeereboom · 2018-11-30T14:29:59Z

politeiawww/backend.go

+	uclr := www.UserCommentsLikesReply{}
+	b.RLock()
+	defer b.RUnlock()
+	for k, v := range b.userLikeActionByCommentID {


This probably needs to go into a map. Do we have an idea how many iterations we do here?

It is a map, this is how it is defined:

// User vote action on each comment userLikeActionByCommentID map[string]map[string]int64 // [token+userid][commentid]action

The number of iterations is equals to the number of comment votes a user has given to a specific proposal.

I am talking about the for loop. What is a normal amount of iterations?

It depends on fo the number of users and proposals. The worst scenario is NU x NP, being NU the total number of users and NP the total number of proposals.

From slack resolution:

fernandoabolafio [3:18 PM] ahh, okay, that makes sense, thanks 2nd one: You asked me how many iterations we could be doing for: `for k, v := range b.userLikeActionByCommentID {...}` So, the worst scenario here is a number of iterations equals to `totalNumberOfUsers x totalNumberOfProposals`. For the current live Pi I would estimate something about 1300 interactions (in the worst case) considering 13 proposals and 100 users. Is that your question? moo31337 [3:31 PM] is this a one time deal? fernandoabolafio [3:33 PM] no, it happens as a result of an user request for their comment likes for a given proposal. E.g every time a logged in user access a proposal route. moo31337 [3:34 PM] so can we not iterate 1300 times? that is an expensive lock fernandoabolafio [3:35 PM] Yeah, I can optimize that

marcopeereboom · 2018-11-30T14:32:24Z

politeiawww/comments.go

+// results and the vote resultant action for the user
+//
+// This function must be called WITH the mutex held
+func (b *backend) _updateResultsForCommentLike(like www.LikeComment) (*www.Comment, error) {


This looks expensive, can we optimize this a bit?

I don't think it is an expensive call. Even though the code looks a bit complex, none of the operations run inside this function is costly. It grabs a value from the inventory, do some pretty basic math calculus and update that value in the inventory. So I think it is okay to make the entire function atomic.
What bits do you see as expensive for this function?

From slack resolution:

fernandoabolafio [3:35 PM] Yeah, I can optimize that okay, so the last one: https://github.com/decred/politeia/pull/610/files#diff-c4d289a7e5b50d4535278b93a68b671aR178 You say that this function `_updateResultsForCommentLike(like www.LikeComment) (*www.Comment, error)` looks expensive. I don’t think it is an expensive call. Even though the code looks a bit complex, none of the operations run inside this function is costly. It grabs a value from the inventory, do some pretty basic math calculus and update that value in the inventory. So I think it is okay to make the entire function atomic. What’s your concern? (edited) moo31337 [3:53 PM] ok i can deal with that

marcopeereboom · 2018-11-30T14:32:54Z

politeiawww/inventory.go

+	commentLikes := ir.commentsLikes
+
+	for _, likes := range commentLikes {
+


No returns here please. Makes it hard to read.

I am bit confused here. Do you want to move the return outside of the loop or remove at all the return value from this function?

No I don't want the whitespace between the for loops.

* Updates signup warning text * Removes paragraph

lukebp reviewed Nov 21, 2018

View reviewed changes

fernandoabolafio mentioned this pull request Nov 21, 2018

Use “comments likes” instead of “comments votes” decred/politeiagui#911

Merged

fernandoabolafio force-pushed the commentvote branch from 398d068 to 36a9296 Compare November 21, 2018 17:05

fernandoabolafio changed the title ~~[d + www]Move comments likes tally from d to www~~ www + d: Move comments likes tally from d to www Nov 21, 2018

fernandoabolafio force-pushed the commentvote branch from 6e847a6 to 443b4a5 Compare November 21, 2018 18:03

lukebp approved these changes Nov 21, 2018

View reviewed changes

marcopeereboom approved these changes Nov 21, 2018

View reviewed changes

marcopeereboom suggested changes Nov 30, 2018

View reviewed changes

move comments likes tally from d to www

eae628c

fernandoabolafio force-pushed the commentvote branch from d455181 to eae628c Compare December 3, 2018 13:26

marcopeereboom approved these changes Dec 3, 2018

View reviewed changes

marcopeereboom merged commit fe4e341 into decred:master Dec 3, 2018

lukebp mentioned this pull request Dec 9, 2018

[www + d] Comments votes score can get messed up as users vote with different identites #582

Closed

vibros68 pushed a commit to vibros68/politeia that referenced this pull request Aug 17, 2021

Updates signup warning text (decred#610)

92c3a38

* Updates signup warning text * Removes paragraph

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

www + d: Move comments likes tally from d to www #610

www + d: Move comments likes tally from d to www #610

fernandoabolafio commented Nov 19, 2018

fernandoabolafio commented Nov 21, 2018

lukebp commented Nov 21, 2018

marcopeereboom Nov 30, 2018

fernandoabolafio Nov 30, 2018 •

edited

marcopeereboom Nov 30, 2018

fernandoabolafio Dec 3, 2018 •

edited

marcopeereboom Nov 30, 2018

fernandoabolafio Nov 30, 2018

marcopeereboom Nov 30, 2018

fernandoabolafio Nov 30, 2018 •

edited

fernandoabolafio Dec 3, 2018

marcopeereboom Nov 30, 2018

fernandoabolafio Nov 30, 2018 •

edited

fernandoabolafio Dec 3, 2018

marcopeereboom Nov 30, 2018

fernandoabolafio Nov 30, 2018

marcopeereboom Nov 30, 2018

		commentLikes := ir.commentsLikes

		for _, likes := range commentLikes {

www + d: Move comments likes tally from d to www #610

www + d: Move comments likes tally from d to www #610

Conversation

fernandoabolafio commented Nov 19, 2018

fernandoabolafio commented Nov 21, 2018

lukebp commented Nov 21, 2018

Choose a reason for hiding this comment

fernandoabolafio Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio Dec 3, 2018 • edited

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

fernandoabolafio Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio Nov 30, 2018 • edited

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

fernandoabolafio Nov 30, 2018 •

edited

fernandoabolafio Dec 3, 2018 •

edited

fernandoabolafio Nov 30, 2018 •

edited

fernandoabolafio Nov 30, 2018 •

edited