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

unused data #517

Closed
hyperloop-rails opened this issue Jun 25, 2018 · 9 comments
Closed

unused data #517

hyperloop-rails opened this issue Jun 25, 2018 · 9 comments

Comments

@hyperloop-rails
Copy link

votes = self.where(
:user_id => user_id,
:comment_id => comment_ids,
)
votes.inject({}) do |memo, v|
memo[v.comment_id] = { :vote => v.vote, :reason => v.reason }

There is unused data retrieval, only comment_id, vote, reason of Vote is used, so the code can be modified to:
votes = self.where( :user_id => user_id, :comment_id => comment_ids, ).select(:comment_id, vote, reason)

@pushcx
Copy link
Member

pushcx commented Jun 26, 2018

This is not the correct refactoring of this code. Zoom out more.

@hyperloop-rails
Copy link
Author

hyperloop-rails commented Jun 28, 2018

Here is the patch

diff --git a/app/models/vote.rb b/app/models/vote.rb
index 4e78e74..e96ac38 100644
--- a/app/models/vote.rb
+++ b/app/models/vote.rb
@@ -72,7 +72,7 @@ class Vote < ActiveRecord::Base
       votes = self.where(
         :user_id    => user_id,
         :comment_id => comment_ids,
-      )
+      ).select(:comment_id, :vote, :reason)
       votes.inject({}) do |memo, v|
         memo[v.comment_id] = { :vote => v.vote, :reason => v.reason }
         memo


@pushcx
Copy link
Member

pushcx commented Jun 28, 2018

This is still not the correct refactoring of the code. I did understand what you suggested but this microoptimization is not the right fix. I was being terse because you sound like a bot or a service expecting free beta testing.

Your paper turned up on Lobsters, that was useful context for this and #518: https://lobste.rs/s/u7srss/how_not_structure_your_database_backed#c_syfieu If you'd like invites to join the conversation, send me an email or DM on twitter. Or if you'd like to grab coffee, I'm on the south side most Tuesdays.

@pushcx
Copy link
Member

pushcx commented Jul 11, 2018

@hyperloop-rails It's been a couple weeks - anyone there?

@hyperloop-rails
Copy link
Author

hyperloop-rails commented Jul 12, 2018

First of all, could you point out which part of the patch is wrong. Since I test the function and the output is the same.
Secondly, Actually we generate this patch is to eliminate the unnecessary data retrieval, as in this example, not used fields are those other than vote, comment_id, reason, so we only retrieve them instead of all fields. In this particular example, I admit the effort is not large enough due to the unused fields didn't occupy that much space. But what we really want to solve is the general cases.
Also, this kind of unnecessary data retrieval should be handled by ORM level instead of developers, which is more efficient and makes the code more readable.

@pushcx
Copy link
Member

pushcx commented Jul 13, 2018

The patch isn't wrong. The patch looks like a bot outputting rules-based fixes rather than a human reading the code, because there's a better solution to this if you look at this function, and another better one if you look at the larger context in the app.

So if you have a bot, great, let's talk about your bot on the site and decide what would be useful. But running the bot and dropping micro-optimizations means you're volunteering me to be the human to spend time thinking about them.

I'm really happy to see the codebase was useful in your studies, because the time contributors spent on it is providing bonus value at no extra work. When you paste bot output and use that as a metric in your next paper as evidence of your bot's value, you're asking contributors to do new work for you. When it's micro-optimizations with no human judgement or even performance metrics, you're providing an extremely marginal benefit to the codebase that is not outweighed by the volunteer time required to do review it.

The problem is not that I don't understand what your patch does or what benefits you claim or what the general class of performance issue is. The problem is that you're not acting like you care about the costs.

@pushcx pushcx closed this as completed Jul 26, 2018
@pushcx
Copy link
Member

pushcx commented Jul 26, 2018

Closed. I'm out of polite things to say about you trying to use me like this.

@hyperloop-rails
Copy link
Author

I feel so sorry to make you feel bad, and I appreciate a lot for your effort in responding to the issues I posted.

Indeed we have a recently published tool (a RubyMine plugin) to help you identify such optimization and do automatic fix, as you can find here: https://hyperloop-rails.github.io/powerstation/, which you can try it on your own Rails project.

I'm not intended to make you feel that I'm using you--I apologize sincerely to hurt feelings--but want to improve the performance problem solving in ORM applications. Helping find out both performance-improving and semantics doable patches is one of our target. And your feedback about the costs concern really helps us a lot in our future work of better tools.

Anyway, I apologize again for my mis-behavior.

@pushcx
Copy link
Member

pushcx commented Jul 26, 2018

You didn't hurt my feelings, you repeatedly neglected to identify yourself as researchers so that professional work me and other maintainers into participating in and evaluating your research project without consent. My feelings aren't hurt by these code patches. For the final time, the problem is your misconduct as an academic researcher.

@lobsters lobsters locked as too heated and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants