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

Reduce much memory usage #405

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

CTKnight
Copy link

@CTKnight CTKnight commented Mar 3, 2017

Android-Week-View is a great library that I really love.
However, I come across a few problem when using this library:

  • too much Calendar.getInstance() during onDraw() method
  • too much new StaticLayout() during onDraw() method
  • too much new float[] during onDraw() method

these codes are not recommended by Android View development document because onDraw is called VERY frequently , so even if allocation of a very small size will be drastically amplified, which leads to frequent GC and GC will stop the drawing process and make the view laggy.

here is my PR:

  1. replace nearly all Calendar.getInstance() by using some private static final Calendar, so no more extra allocation during onDraw()
  2. add these fields to record the drawing status, if the height of EventRec is not changed, the old StaticLayout will be retained and used.
private int mAvailableHeight;
private int mAvailableWidth;
private StaticLayout mTextLayout;
private SpannableStringBuilder mStringBuilder

I have try my best to not to modify the API, It just take 4 lines change in the demo.

@AndroidDeveloperLB
Copy link

You can also use LocalDate and LocalDateTime , which are faster and use less memory.
Sadly, they are available only from API 26 (Android O), but there are ways to use it via libraries:

@CTKnight
Copy link
Author

CTKnight commented May 6, 2018

I love ThreeTenBP and I've already utilized it in my app, since it is an excellent backport for Java 8 time API. However, it is based on immutable classes as Java 8 time API documents. So it's not appropriate to be used in onDraw method that would be called many times per second, as it would return new instances everytime it's modified, which put much pressure on GC.

@AndroidDeveloperLB
Copy link

I don't know about onDraw, but it uses much less memory and is faster in terms of performance, at least in my tests...

You can combine your way, with usage of LocalDate and LocaleDateTime.

See here:

image

Code:

    AndroidThreeTen.init(this)
    val calsList = ArrayList<java.util.Calendar>()
    val localDateList = ArrayList<LocalDate>()
    object : Thread() {
        override fun run() {
            run {
                var startTime = System.currentTimeMillis()
                for (i in 0..1000) {
                    calsList.add(Calendar.getInstance())
                }
                var timeTaken = System.currentTimeMillis() - startTime
                Log.d("AppLog", "time taken for calendars list:$timeTaken")
            }
            run {
                var startTime = System.currentTimeMillis()
                for (i in 0..1000) {
                    localDateList.add(LocalDate.now())
                }
                var timeTaken = System.currentTimeMillis() - startTime
                Log.d("AppLog", "time taken for localDate list:$timeTaken")
            }
        }
    }.start()

@CTKnight
Copy link
Author

CTKnight commented May 6, 2018

Ok, I will do a more detailed benchmark on this. The point here actually isn't lying in Calendar or ThreeTenBP, it's the the newing instance behavior in onDraw method. Many instances are created, so GC will be busy clearing these instances and lag happens. What I did in this PR is caching the time instance in term of long primitive value which is much cheaper than objects (yes, LocalDate can do this, but not necessary though, and this lib uses Calendar everywhere, moving this lib from Calendar to threetenbp is another topic). And I observed a relief for frequent GC in my fork version and less laggy in my app.

@CTKnight CTKnight closed this May 6, 2018
@CTKnight CTKnight reopened this May 6, 2018
@AndroidDeveloperLB
Copy link

You can also check the memory usage.

Of course, the above test is for LocalDate. In some cases, you would want to use LocalDateTime instead. Depends on the case.

Also, sadly, the library requires initialization , but as I've found out, you can avoid it in many cases, as long as you don't need times zones (the initialization is of time zones loading).

In the current library of viewing a calendar, it's not needed to load zones, so I'm very sure you can avoid the initialization.

@CTKnight
Copy link
Author

CTKnight commented May 6, 2018

Timezone supporting is in another PR though. see #272

@AndroidDeveloperLB
Copy link

Do you think there should be a time zone here?
Is it because of the daylight-saving?

@CTKnight
Copy link
Author

CTKnight commented May 6, 2018

Ah, I mean timezone things may be discussed in that PR. This PR just focuses on performance improvement and reducing API change as much as possible. And thr purposal for using ThreeTenBP doesn't seem to help improving performance bottleneck much in this PR (see my comment edited above), maybe you can open another PR for using ThreeTenBP?

@AndroidDeveloperLB
Copy link

To me I don't see performance issues yet, and my main tasks is to make it as similar as possible to what the designer told, together with snapping by week.

I've made a pull request here, including some improvements and even conversion to Kotlin :
#496

But it seems this library is actually not maintained anymore. Do you know perhaps how to set it to scroll horizontally with snapping of a whole week?
It was asked here, but I don't see a solution.

I've also tried to go over all of the forks, but none had it. It seems this fork is most maintained currently, but it doesn't have it, and it actually has scrolling issue (reported here), yet is fixes another (here)

@CTKnight
Copy link
Author

CTKnight commented May 6, 2018

I got ur idea then, u mean scrolling from week to week? This is a feature in my app, I use a work around:

  1. Modify the source of week view and disable horizontal swipping in the view(this part is not in my fork)
  2. Place a weekview per fragment set it to display 7 days events, use a fragment page adapter to achieve smooth horizontal swipping.

@AndroidDeveloperLB
Copy link

AndroidDeveloperLB commented May 6, 2018

@CTKnight See my answer here:
#11 (comment)

@AndroidDeveloperLB
Copy link

Say, can you please set the new changes on my fork? It just got quite different from original repository, as I've converted it to Kotlin and all...
Here's the link:
Quivr#97 (comment)

@AndroidDeveloperLB
Copy link

Weird thing is that even when I clone your repository, it still is quite slow on old devices (Nexus 4 with Android 4.4.4) which run Google Calendar app just fine.
I don't get what's causing it to be so slow on such cases.

@CTKnight
Copy link
Author

@AndroidDeveloperLB I'm afraid I'm not available for this until September... As to the performance, this PR only remedies the allocation of Calendar class, but the StaticLayout class as mentioned is not included, so there is still allocation in onDraw method though it's much less than Calendar class.

@AndroidDeveloperLB
Copy link

Well it's a start...
I'm sure there are plenty of things to optimize here.
I wish the code wasn't so complex and long.

@jlurena
Copy link

jlurena commented Jul 15, 2018

I have a PR using Android 26, removed all java.util.Calendar usage and replaced with java.time. Please take a look

Some work still needs to be done, but for my purposes it's enough.

Quivr#110

@AndroidDeveloperLB
Copy link

AndroidDeveloperLB commented Jul 15, 2018

Oh too bad. You forked from Java instead of what I did here... Then again, I don't think I told you about it.

Also, it's not recommended to use the built in class of LocalDateTime, because it requires a very new Android API version. You could have used one of the libraries I've mentioned here .

How can I try what you've made? clone this : https://github.com/jlurena/Android-Week-View ?

@jlurena
Copy link

jlurena commented Jul 15, 2018

@AndroidDeveloperLB yep, and no I wasn't aware. I just saw this issue earlier today :\

And yes I understand that. The app I'm working on however will only work with devices with Android >= 26

@AndroidDeveloperLB
Copy link

I have made tons of changes, fixes, and improvements.
But I didn't change the use of Calendar class. A bit less instances, but still being used a lot.
I recommend to try it out.

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

Successfully merging this pull request may close these issues.

3 participants