29

There is a source file in a rather large project with several functions that are extremely performance-sensitive (called millions of times per second). In fact, the previous maintainer decided to write 12 copies of a function each differing very slightly, in order to save the time that would be spent checking the conditionals in a single function.

Unfortunately, this means the code is a PITA to maintain. I would like to remove all the duplicate code and write just one template. However, the language, Java, does not support templates, and I'm not sure that generics are suitable for this.

My current plan is to write instead a file that generates the 12 copies of the function (a one-use-only template expander, practically). I would of course provide copious explanation for why the file must be generated programmatically.

My concern is that this would lead to future maintainers' confusion, and perhaps introduce nasty bugs if they forget to regenerate the file after modifying it, or (even worse) if they modify instead the programmatically-generated file. Unfortunately, short of rewriting the whole thing in C++, I see no way to fix this.

Do the benefits of this approach outweigh the disadvantages? Should I instead:

  • Take the performance hit and use a single, maintainable function.
  • Add explanations for why the function must be duplicated 12 times, and graciously take the maintenance burden.
  • Attempt to use generics as templates (they probably don't work that way).
  • Yell at the old maintainer for making code so performance-dependent on a single function.
  • Other method to maintain performance and maintainability?

P.S. Due to the poor design of the project, profiling the function is rather tricky... however, the former maintainer has convinced me that the performance hit is unacceptable. I assume by this he means more than 5%, though that is a complete guess on my part.


Perhaps I should elaborate a bit. The 12 copies do a very similar task, but have minute differences. The differences are in various places throughout the function, so unfortunately there are many, many, conditional statements. There are effectively 6 "modes" of operation, and 2 "paradigms" of operation (words made up by myself). To use the function, one specifies the "mode" and "paradigm" of operation. This is never dynamic; each piece of code uses exactly one mode and paradigm. All 12 mode-paradigm pairs are used somewhere in the application. The functions are aptly named func1 to func12, with even numbers representing the second paradigm and odd numbers representing the first paradigm.

I'm aware that this is just about the worst design ever if maintainability is the goal. But it seems to be "fast enough", and this code hasn't needed any changes for a while... It's also worth noting that the original function has not been deleted (although it is dead code as far as I can tell), so refactoring would be simple.

11
  • If the performance hit is serious enough to warrant 12 versions of the function, then you might be stuck with it. If you refactor to a single function (or use generics), would the performance hit be bad enough that you'll lose customers and business? Jun 24, 2014 at 18:06
  • 12
    Then I think you need to do your own tests (I know you said profiling is tricky, but you can still do rough "black-box" performance tests of the system as a whole, right?) to see just how big a difference there is. And if it's noticable, then I think you might be stuck with the code generator. Jun 24, 2014 at 18:11
  • 1
    This could sounds like it could have benefited from some parametric polymorphism (or maybe even generics), rather than calling each function by a different name. Jun 24, 2014 at 20:18
  • 2
    Naming the functions func1 ... func12 seems insane. At least name them mode1Par2 etc... or maybe myFuncM3P2.
    – user949300
    Jun 25, 2014 at 5:53
  • 2
    " if they modify instead the programmatically-generated file"... create the file only when building from the "Makefile" (or whatever system you use) and remove it right aftter compilation finished. In this way they simply don't have a chance to modify the wrong source file.
    – Bakuriu
    Jun 25, 2014 at 11:06

10 Answers 10

23

This is a very bad situation, you need to refactor this ASAP - this is technical debt in it's worst - you don't even know how important the code really is - only speculate that it's important.

As to solutions ASAP:
Something that can be done is adding a custom compilation step. If you use Maven that is actually fairly simple to do, other automated build systems are likely to cope with this as well. Write a file with a different extension than .java and add a custom step that searches your source for files like that and regenerates the actual .java. You may also want to add a huge disclaimer on the auto-generated file explaining not to modify it.

Pros vs using a once-generated file: Your developers will not get their changes to the .java working. If they actually run the code on their machine before committing they will find that their changes have no effect (hah). And then maybe they will read the disclaimer. You are absolutely right in not trusting your teammates and your future self with remembering that this particular file has to be changed in a different way. It also allows automatic testing as wel, as JUnit will compile your program before running tests (and regenerate the file as well)

EDIT

Judging by the comments the answer came off as if this is a way to make this work indefinitely and maybe OK to deploy to other performance critical parts of your project.

Simply put: it is not.

The extra burden of creating your own mini-language, writing a code-generator for it and maintaining it, not to mention teaching it to future maintainers is hellish in the long run. The above only allows a safer way to handle the problem while you are working on a long-term solution. What that will take is above me.

10
  • 19
    Sorry, but I have to disagree. You don't know enough about the OP's code to make this decision for him. Code-generating this seems to me like it contains more technical debt than the original solution does. Jun 24, 2014 at 20:17
  • 6
    Robert's comment cannot be overstated. Any time you have a "disclaimer explaining not to modify a file" that is a the "technical debt" equivalent of a check-cashing store run by an illegal bookie nicknamed "Shark".
    – corsiKa
    Jun 24, 2014 at 20:37
  • 8
    The auto-generated file of course doesn't belong in the source repository (it's not source, after all, it's compiled code) and it should be removed by ant clean or whatever your equivalent is. No need to put a disclaimer in a file that isn't even there! Jun 24, 2014 at 21:19
  • 2
    @RobertHarvey I am not making any decisions for the OP. OP asked whether it is a good idea to have such templates in the way he has them and I proposed a (better) way to maintain them. It is a not an ideal solution and, in fact, as I have stated in the very first sentence, the whole situation is bad, and a much better way to go forward is to remove the problem, not make a shaky solution to it. That includes properly assessing just how critical is this code, how bad is the performance hit and whether there are ways to make it work without writing a lot of bad code.
    – Ordous
    Jun 24, 2014 at 21:58
  • 2
    @corsiKa I never said this is a lasting solution. This would be as much of a debt as the original situation. The only thing it does is decrease volatility until a systematic solution is found. Which will likely include moving entirely to a new platform/framework/language since if you're hitting problems like this in Java - you're either doing something extremely complex or extremely wrong.
    – Ordous
    Jun 24, 2014 at 22:02
16

Is the maintenance hit real, or does it just bother you? If it only bothers you, leave it alone.

Is the performance issue real, or did the previous developer only think it was? Performance problems, more often than not, are not where they are thought to be, even when a profiler is used. (I use this technique to reliably find them.)

So there is a possibility that you have an ugly solution to a non-problem, but it could also be an ugly solution to a real problem. When in doubt, leave it alone.

10

Really make sure than under real, production, conditions (realistic memory consumption, that triggers Garbage Collection realistically, etc), all those separate methods really make a performance difference (as opposed to having only one method). This will take a few days out of your time, but may save you weeks by simplifying the code you work with from now on.

Also, if you do discover that you need all the functions, you might want to use Javassist to generate the code programatically. As other have pointed out, you can automate it with Maven.

1
  • 2
    Also, even if it turns out that the performance issues are real, the exercise of having studied them will help you to better know what is possible with your code. Jun 25, 2014 at 0:54
6

Why not include some sort of template preprocessor\code generation for this system? A custom extra build step that executes and emits extra java source files before compiling the rest of the code. This is how including web clients off of wsdl and xsd files often works.

You'll of course have the maintenance of that preprocessor\code generator, but you won't have the duplicated core code maintenance to worry about.

Since compile time Java code is being emitted, there is no performance penalty to be paid for extra code. But you gain maintenance simplification by having the template instead of all of the duplicated code.

Java generics give no performance benefit due to type erasure in the language, simple casting is used in its place.

From my understanding of C++ templates, they are compiled into several functions per each template invocation. You'll wind up with duplicated mid-compile time code for each type you store in a std::vector for example.

2

No sane Java method can be long enough to have 12 variants... and JITC hates long methods - it simply refuses to optimize them properly. I've seen a speed up factor of two by simply splitting a method into two shorter ones. Maybe this is the way to go.

OTOH having multiple copies may make sense even if there were identical. As each of them gets used in different places, they get optimized for different cases (the JITC profiles them and then places the rare cases on an exceptional path.

I'd say that generating code is no big deal, assuming there's a good reason. The overhead is rather low, and properly named files lead immediately to their source. Some long time ago as I generated source code, I put // DO NOT EDIT on every line... I guess that's save enough.

1

There is absolutely nothing wrong with the 'problem' you mentioned. From what I know this is the exact kind of design and approach used by DB server to have good performance.

They have lot of special methods to make sure they can maximize the performance for all kinds of operations: join, select, aggregate, etc... when certain conditions apply.

In short, code generation like the one you think is a bad idea. Perhaps you should look at this diagram to see how a DB solves a problem similar to yours:enter image description here

1

You might want to check whether you can still use some sensible abstractions and use e.g. the template method pattern to write understandable code for the common functionality and move the differences between the methods into the "primitive operations" (according to the pattern descrition) in 12 subclasses. This will mightily improve maintainability and testability, and it might actually have the same performance as the current code, since the JVM can inline the method calls for the primitive operations after a while. Of course, you have to check this in a performance test.

0

You can solve the problem of specialized methods by using Scala language.

Scala can inline methods, this (in combination with easy higher order function usage) makes it possible to avoid code duplication for free — this looks like the main problem mentioned in your answer.

But also, Scala has syntactic macros, which makes it possible to do a lot of things with code at compile time in a type-safe manner.

And the common problem of boxing primitive types when used in generics is possible to solve in Scala, too: it can do generic specialization for primitives to avoid boxing automatically by using @specialized annotation — this thing is built into language itself. So, basically, you'll write one generic method in Scala, and it will run with the speed of specialized methods. And if you need fast generic arithmetics too, it's easily doable by using Typeclass "pattern" for injecting the operations and values for different numeric types.

Also, Scala is awesome in many other ways. And you don't have to rewrite all code to Scala, because Scala <-> Java interoperability is excellent. Just make sure to use SBT (scala build tool) for building the project.

-1

If the function in question is large, turning the "mode/paradigm" bits into an interface and then passing an object that implements that interface as a parameter to the function can work. GoF calls this the "Strategy" pattern, iirc. If the function is small, the increased overhead may be significant... did somebody mention profiling yet?... more people should mention profiling.

1
  • this reads more like a comment than an answer
    – gnat
    Jun 25, 2014 at 8:58
-2

How old is the program? Most probably newer hardware would remove the bottleneck and you could switch to a more maintainable version. But there needs to be a reason for maintenance, so unless your task is to improve the codebase, leave it be as it is working.

2
  • 1
    this seems to merely repeat points made and explained in a prior answer posted few hours ago
    – gnat
    Jun 24, 2014 at 21:05
  • 1
    The only way to determine where a bottleneck actually is is to profile it - as mentioned in the other answer. Without that key information, any suggested fix to the performance is pure speculation.
    – user40980
    Jun 24, 2014 at 22:22

Not the answer you're looking for? Browse other questions tagged or ask your own question.