this post was submitted on 17 Nov 2023
50 points (90.3% liked)

Programming

17579 readers
137 users here now

Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!

Cross posting is strongly encouraged in the instance. If you feel your post or another person's post makes sense in another community cross post into it.

Hope you enjoy the instance!

Rules

Rules

  • Follow the programming.dev instance rules
  • Keep content related to programming in some way
  • If you're posting long videos try to add in some form of tldr for those who don't want to watch videos

Wormhole

Follow the wormhole through a path of communities [email protected]



founded 2 years ago
MODERATORS
all 23 comments
sorted by: hot top controversial new old
[–] [email protected] 15 points 1 year ago* (last edited 1 year ago)

Seems more applicable to an imperative style, and IMO even still the advice is too dependent on special/actual case details to be generally applicable as a "rule of thumb".

This is just one specific example amongst many of how redundant logic could be simplified because sometimes the branch is an implementation detail and you want to push it down, and sometimes it's not and you want to push it up.

[–] [email protected] 12 points 1 year ago

I disagree with this article.

The first one is a good point in principle, but I would call it pushing down the ifs. A method is usually used to group logic together. So pulling the assertion checks out of that method forces you to replicate it every single time before calling the function. It's very likely you forget to update these conditions in one place and not worth the miniscule loss of performance.

Now, if frobnicate is only something a walrus can do, then it should take a walrus. But if it is something like "given a picture of an animal, check if it is of a walrus, determine its sex and if it is a male, then give the likelihood that is the dominant male on this part of the beach" then you should most certainly push the ifs down to contain all that logic in one single frobnicate method.

Regarding the loops:

If the example given is really all the code you have, then sure, pull that condition out of the loop if you want. But the compiler will optimize it anyway if the condition does not change every loop iteration. And if it does you can't pull the condition out of the loop anyway. A for loop I have much more often in my code looks like this though:

//BAD?
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

    if study.is_about_frovnication { 
        walrus.frobnicate() 
    } else { 
        walrus.transmogrify()
    } 
} 

Which you do not want to spread to two for loops! Sure, in this toy example the if conditions could be simplified. And if you have such trivial loops, you should use iterators anyway. But sometimes you have logic you need to apply to every element, do not want to put that logic in the frobnicate method (especially if you push your ifs up) and can't use iterators in an elegant way.

For the love of god, do not turn it into

if study.is_about_frobnication {
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

walrus.frobnicate()
} else {
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

    walrus.transmogrify()
}

And last but not least, Profile before you optimize anything. The part about the ifs, and the enum example may not actually impact your performance at all! The compiler is very smart about moving code around, so the enum thing would most likely be inlined and then reduced down further, potentially eliminating the enum from the code completely. It can still be a worthwhile code change to make, because it improves legibility. But since the author mentioned the performance a few times I wanted to give a counterpoint. (That being, profile before you optimize!)

[–] onlinepersona 8 points 1 year ago (1 children)

I agree with part of the article, because I didn't read the rest. I truly dislike the use of single letter variable names: f, g, h and foo, bar, baz. My advice: use descriptive variable names.

function twoIfs, function complicatedIf, var simpleAnd, etc. Makes it so much easier to read examples instead of remembering "oh yeah, f had two ifs in it, h had the if/else, g calls f which calls h which,...".

Also see this often in other examples: "A for 'Truthy variable' " 😓 Wtf. Laziness is good when it makes things easier, not harder.

[–] lysdexic -5 points 1 year ago (2 children)

My advice: use descriptive variable names.

The article is really not about naming conventions.

[–] Sheldan 14 points 1 year ago (3 children)

Should have still used them. It was harder to read this way.

[–] [email protected] 1 points 1 year ago

Agreed that some people can find it easier with explicit names - however some people find it easier with short meaningless names as it makes them focus on the abstraction rather than the naming. There is no right or wrong here. It all depends on the reader.

[–] sukhmel 1 points 1 year ago

I even thought that this (hardness) was intended to emphasize the way it's hard to spot problems in real codebase 😅

[–] onlinepersona 13 points 1 year ago

Doesn't matter, it's hard to read an article. If it were hard to read for another reason like bad grammar, I'd comment on that too 🤷

[–] [email protected] 3 points 1 year ago (4 children)

I'm curious, is there a difference in doing it like this?:

var condition = var1 && var2

for walrus in walruses {
  if (condition) {
    walrus.frobnicate()
  } else {
    walrus.transmogrify()
  }
}

Doesn't assigning the condition to a variable make it so that it's only really evaluated once?

[–] nik9000 12 points 1 year ago (1 children)

I'm reasonably sure compilers can shift the if out. I believe it's called "loop invariant code motion". Won't work in call cases, but in the variable case it should.

[–] lysdexic 0 points 1 year ago (1 children)

I’m reasonably sure compilers can shift the if out. I believe it’s called “loop invariant code motion”.

That scenario would only apply if the condition was constant and specified at compile time. There's no indication on whether var1 or var2 are compile-time constants or predicates evaluated at runtime.

[–] nik9000 9 points 1 year ago (1 children)

I don't believe you have to specify the condition at compile time. I think that optimization would fall under dead code elimination.

For the invariant code motion stuff the comparison just has to be invariant from start to finish. At least, that's been my experience. The compiler will just shift the if stement.

But, like, there are totally times when it can't figure out that the thing is invariant. And sometimes it's just more readable to move the if statement out of the loop.

[–] lysdexic 0 points 1 year ago (1 children)

I don’t believe you have to specify the condition at compile time. I think that optimization would fall under dead code elimination.

How do you tell if some code behind a conditional is dead if the predicate that drives the condition is evaluated at runtime?

[–] nik9000 4 points 1 year ago (1 children)

Sorry. I wasn't clear. If the conditional is constant a compile time you get the dead code optimization. The path not taken is removed. If it's not constant at you may get the loop invariant movement. But only if the compiler can tell that it's invariant.

My point wasn't that you should always rely on this behavior. At least, I didn't mean to say that. I suppose what I should have said is more like "in many cases you won't see any performance difference because the compiler will do that for you anyway."

I suppose I have value judgements around that like "generally you should do the thing that is more readable and let the compiler take care of stuff like moving the loop invariant". That's been mostly true for me. But only mostly.

[–] lysdexic -2 points 1 year ago* (last edited 1 year ago) (1 children)

If it’s not constant at you may get the loop invariant movement. But only if the compiler can tell that it’s invariant.

The point is that if the predicate is evaluated at runtime then the compiler plays no role because there is no compile-time constant and all code paths are deemed possible.

I suppose what I should have said is more like “in many cases you won’t see any performance difference because the compiler will do that for you anyway.”

I understand that you're trying to say that compilers can leverage compile-time constants to infer if code paths are dead code or not.

That's just a corner case though. Your compiler has no say on what code paths are dead if you're evaluating a predicate from, say, the response of a HTTP request. It doesn't make sense to expect this hypothetical scenario to be realistic when you have no info on where a predicate is coming from.

[–] Lmaydev 3 points 1 year ago* (last edited 1 year ago)

They're talking about moving the calculation of the if condition outside of the loop. Which the original commenter mentioned. https://en.m.wikipedia.org/wiki/Loop-invariant_code_motion

You are the one that brought up dead code elimination.

[–] coloredgrayscale 3 points 1 year ago* (last edited 1 year ago)

The variable is set once, but the if expression is still evaluated every time (unless the compiler can optimize it)

(edit after skimming the article: yes,using the variable would solve the problem of the last example)

So there would be the branching overhead in every iteration. But that's something the cpu branch prediction should cover, especially since the taken branch will be identical in every loop.

Same also applied to the implied condition to break the for loop (only the first few and last iteration should be wrong predictions)

[–] [email protected] 1 points 1 year ago (1 children)

You still need to read the variable in each iteration, but the cost of that is probably negligible

[–] declination 4 points 1 year ago* (last edited 1 year ago)

Modern optimizing compilers are magical. I would need to check assembly but I would actually expect the if to be hoisted out of the loop entirely to relieve pressure on the branch predictor.

[–] UndercoverUlrikHD 1 points 1 year ago

What's up with that capitalisation, I thought it was about git lfs at first, really confused me lol

[–] Anders429 -1 points 1 year ago

Saw this posted on hackernews yesterday, along with hundreds of comments of people completely misunderstanding the advice given. Glad to not see any of that here.