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

[FLINK-2471]Improve File Sink #973

Closed
wants to merge 1 commit into from
Closed

Conversation

ffbin
Copy link
Contributor

@ffbin ffbin commented Aug 3, 2015

FileSinkFunction invoke funtion always call updateCondition, so we can use a variable updateTime to avoid frequent operate (System.currentTimeMillis() - lastTime).

@gyfora
Copy link
Contributor

gyfora commented Aug 3, 2015

I don't see that you avoid calling System.currentTimeMillis, you remove one addition and you also add one.

@StephanEwen
Copy link
Contributor

Arithmetic (addition subtraction) in a pipeline is incredibly cheap on today's processors, to this is probably micro optimization ;-)

@ffbin
Copy link
Contributor Author

ffbin commented Aug 3, 2015

In one millisecond, it will call updateCondition about 10000 times and it will do minus operate 10000 times.But update lastTime is less than times of call updateCondition.So i add updateTime to reduce call times of minus operate.

@StephanEwen
Copy link
Contributor

The minus operation is not really evaluated in isolation. It is part of the processor instruction pipeline at that point (see superscalar execution architectures). It will most likely be computed in flight with other instructions without adding overhead, because the pipeline's throughput is at that point not limited by the ALU. Judging the cost or an isolated operation does not work any more on modern processors.

The processing time is in almost all cases dominated stalls in the pipeline doe to fetching variables from memory.

@ffbin
Copy link
Contributor Author

ffbin commented Aug 3, 2015

hi, I can run successfully in local computer,but CI is failed,How can i see why CI is failed?
I can not see the details content ,it is "Do you have a question?".

@StephanEwen
Copy link
Contributor

Can you access this link here? https://travis-ci.org/apache/flink/builds/73886757

@StephanEwen
Copy link
Contributor

BTW: I am not saying we should not merge this. It is actually a fine fix.

I am only trying to help judge the importance of issues, to help deciding what would be important to fix.

@StephanEwen
Copy link
Contributor

If you want to contribute some performance improvements, how about bundling them per component. Something like "Improve File Sink" pull request, where all fixes to the FileSinkFunction go together.

@ffbin
Copy link
Contributor Author

ffbin commented Aug 4, 2015

Hi , i can not access https://travis-ci.org/apache/flink/builds/73886757. Open it , i can only see like this:
image

@ffbin
Copy link
Contributor Author

ffbin commented Aug 4, 2015

hi, System.currentTimeMillis() is cost performance very much.I want to use a thread to call System.currentTimeMillis and update a static long variable millTime.All other module do not need call System.currentTimeMillis() and can use millTime directly.I have test it can improve performance very much.

@ffbin ffbin changed the title [FLINK-2471]FileSinkFunction invoke performance optimize [FLINK-2471]Improve File Sink Aug 7, 2015
@StephanEwen
Copy link
Contributor

Can we close this pull request? I think

The followup idea if adding a "TimeService" with a thread that avoid too frequent calls to System.currentTimeMillis() is an interesting idea...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants